Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFE] default_suite configuration #68

Open
NickLaMuro opened this issue Jan 19, 2021 · 11 comments
Open

[RFE] default_suite configuration #68

NickLaMuro opened this issue Jan 19, 2021 · 11 comments
Assignees
Labels
enhancement New feature or request

Comments

@NickLaMuro
Copy link
Member

This is really a two fold feature request:

  • Be able to define what test suite (rake command?) is run for a given repo
  • Be able to configure a default_suite(s) for each repo (if it different than the default.

I have a lot of ideas on how we could do this, but to start, it might make sense to just have a test_suite_overrides.yml (name pending) where it will override what rake task is used when a given repo is run.

manageiq-ui-classic for example probably wants the javascript tests run instead of just the rake spec task. That said, even just that poses problems because then we also have to consider any setup to make sure that the npm/node dependencies are in place, which will require some other features to be in place for this to function.

My thoughts on this is that it would probably be easier to just move scripts like setup.sh into rake tasks, so that we can use prerequisite tasks to make the following easier to make custom for each repo as needed:

system!({"TRAVIS_BUILD_DIR" => test_repo.path.to_s}, "bash", "tools/ci/before_install.sh") if ENV["CI"] && File.exist?("tools/ci/before_install.sh")
system!(env_vars, "bin/setup")
system!(script_cmd)

But that might be a minority preference from myself.


There is probably more to this request then what I have outlined above, but I think it is something to consider as manageiq-ui-classic was a perfect example in the Rails 6 upgrade where it was a complete blind spot. Specifically because we didn't test the javascript perspective, when it was a factor in allowing the test suite to pass properly once the effort was merged.

(though, in all fairness, the app did seem to boot just fine, just that the tests weren't functioning as a result, and became worthless).

@agrare
Copy link
Member

agrare commented Jan 19, 2021

Yeah I think this is a great suggestion, started thinking about it after not running the javascript specs for Rails 6

As for #1

What if we add a --test-suite= command-line option and TEST_SUITE env var to manageiq-cross_repo command, and the default script_cmd = "bundle exec rspec $TEST_SUITE" This would naturally allow for e.g. manageiq-cross_repo --test-repo=manageiq-ui-classic --test-suite=spec:javascript to turn into bundle exec rake spec:javascript

When calling from a manageiq-cross_repo-tests .travis.yml it can be - TEST_REPO=manageiq-ui-classic TEST_SUITE=spec:javascript

That leaves how to call this easily from miq-bot, we could have some separator to indicate the test_suite requested like: @miq-bot cross-repo-tests manageiq-ui-classic$spec:javascript

That doesn't cover 100% of what you're looking to do, especially the "default suites" which would also be really nice (like always run spec and spec:javascript for ui-classic).

@NickLaMuro
Copy link
Member Author

NickLaMuro commented Jan 19, 2021

What if we add a --test-suite= command-line option and TEST_SUITE env var ...

@agrare For the most part, I think we already have that:

https://github.com/ManageIQ/manageiq-cross_repo/pull/64/files

The problem, however, lies that for manageiq-ui-classic, there is a large amount of setup required to actually install node itself as part of the setup.sh type stuff:

Both of which, are really needed to actually run the JS specs. To complicate matters, nvm is a bash function, not a script... so it isn't really something that we can just call out to from rake (as much as I would like to...).

@agrare
Copy link
Member

agrare commented Jan 20, 2021

For the most part, I think we already have that:

Right you could add SCRIPT_CMD="bundle exec rake TEST_SUITE=spec:javascript" already, I was thinking we could simplify things greatly if what you want to do is run SCRIPT_CMD="bundle exec rake TEST_SUITE=spec and SCRIPT_CMD="bundle exec rake TEST_SUITE=spec:javascript by only passing the different test_suite.

Is there a reason why most of that setup couldn't be done in bin/setup ?

@NickLaMuro
Copy link
Member Author

@agrare because bin/setup has never been for installing a system dependency? (such as node/ruby)

@agrare
Copy link
Member

agrare commented Jan 20, 2021

We do call tools/ci/before_install.sh to install things like e.g. libqpid-proton though

@NickLaMuro
Copy link
Member Author

Again, doesn't cover before_script.sh, which I linked above for manageiq-ui-classic, and is necessary for the tests to run properly (and isn't part of the bin/setup, because it is a bunch of extra time that is only necessary for the JS specs).

If you want to just see what I am talking about, just try running the javascript tests for manageiq-ui-classic. I am pretty sure I spent about a day trying to get it to work (granted, I also was probably trying to refactor things a bit as well, so that time is also included), and I think it ended in me just giving up.

@agrare
Copy link
Member

agrare commented Jan 20, 2021

Just threw an example together but what about something like #69
It'll run any before_install/before_script/after_script that is defined in the .travis.yml so it calls the before_script.sh in the ui-classic repo.

@NickLaMuro
Copy link
Member Author

@agrare Oh, I do like this idea. Let me throw together a cross repo run for you to text this out.

@agrare
Copy link
Member

agrare commented Feb 1, 2021

Okay so with #69 you can give a TEST_SUITE env var in the .travis.yml

I think the best next step would be to add a way to do this with @miq-bot cross-repo-tests just need to come up with a good syntax for it.

Thinking maybe @miq-bot cross-repo-tests manageiq-ui-classic::spec:javascript or manageiq-ui-classic~spec:javascript

It has to be something that we don't use for branch/pr/sha so @ and # are out and : is commonly used for the suite name separator

@NickLaMuro
Copy link
Member Author

@agrare maybe just have it be a comma delimited list in parenthesis following the previous syntax?

@miq-bot cross-repo-tests manageiq-ui-classic#123(spec,spec:javascript,spec:compile)

@miq-bot cross-repo-tests manageiq-ui-classic@jansa(spec,spec:javascript,spec:compile)

@Fryguy
Copy link
Member

Fryguy commented Feb 2, 2021

I like @NickLaMuro 's suggestion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants