-
Notifications
You must be signed in to change notification settings - Fork 117
[feat] Introduce a --dry-run option
#2746
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
Conversation
Codecov ReportBase: 86.61% // Head: 86.62% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## develop #2746 +/- ##
========================================
Coverage 86.61% 86.62%
========================================
Files 60 60
Lines 11215 11259 +44
========================================
+ Hits 9714 9753 +39
- Misses 1501 1506 +5
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides my individual comments in the code, I think the proposed changes are very intrusive. I don't think we should touch the scheduler backends. The dry-run mode mechanics should stop in the RegressionTest. If the test is in dry-run mode, we should simply skip the job submission. Similarly, the compile|run_wait() should return immediately and the compile|run_complete() should always return True. As for the sanity and performance phases, I suggest that we enter normally in those stages and exit just before we start the evaluation of the sanity or performance functions. That would allow to reveal test problems even in dry-run mode. Finally, we should probably omit completely the creation of the output directory as nothing will be eventually copied there and the "cleanup" stage should just return immediately. The idea of the dry-run mode is primarily to be able to inspect the generated scripts, which will be left in the stage dir. I would also claim that the copy from sourcesdir to the stagedir should still happen, as it will also reveal potential problems without actually running the test.
|
@ekouts The PR is now fine from my side. |
ekouts
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot approve because it's my PR, but @vkarak lgtm 🎉
--dry-run option
Adding a "dry-run" attribute instead of a cli option gives to the user more flexibility, in case they want to run a fixture/dependency for some reason. The user can always run with
-Sdry_run_mode=1so that they don't actually change the test in most cases.I also changed the color of the "OK" and "RUN" messages to yellow for tests that have
dry_run_mode=Truebut we can easily change it back if you don't like it. I just thought that this feature can easily lead to mistakes if someone is setting the test indry_run_modewithout noticing.Closes #2242.