Skip to content

Conversation

@ekouts
Copy link
Contributor

@ekouts ekouts commented Feb 14, 2020

Fix CompileOnlyRegressionTest setup hooks.

  • Fix should be simple for the setup hooks, I just also need to find where to add a unittest for that.
  • The pre_run hook in RunOnlyRegressionTest is not called exactly before the run but only because there is a call to the base class function.
  • Finally I don't know if we want the hooks to be called in no-op functions, like the run in CompileOnlyRegressionTest. With the current implementation they won't be called.

Fixes issue #1162.

@ekouts ekouts added this to the ReFrame sprint 20.02 milestone Feb 14, 2020
@ekouts ekouts self-assigned this Feb 14, 2020
@ekouts ekouts requested review from teojgo and vkarak February 14, 2020 08:05
@ekouts ekouts linked an issue Feb 14, 2020 that may be closed by this pull request
@ekouts
Copy link
Contributor Author

ekouts commented Feb 14, 2020

@jenkins-cscs retry tsa

@vkarak vkarak changed the title [bugfix] Fix CompileOnlyRegressionTest setup hooks [wip] [bugfix] Fix CompileOnlyRegressionTest setup hooks Feb 14, 2020
Copy link
Contributor

@vkarak vkarak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will need a unit test and a fix for the run() of the run only tests. But this is not blocking us for the moment, so we can push it to the next sprint.

@ekouts ekouts changed the title [wip] [bugfix] Fix CompileOnlyRegressionTest setup hooks [bugfix] Fix CompileOnlyRegressionTest and RunOnlyRegressionTest hooks Mar 2, 2020
@codecov-io
Copy link

Codecov Report

Merging #1163 into master will decrease coverage by 0.01%.
The diff coverage is 97.59%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1163      +/-   ##
==========================================
- Coverage   92.17%   92.15%   -0.02%     
==========================================
  Files          81       81              
  Lines       12001    12074      +73     
==========================================
+ Hits        11062    11127      +65     
- Misses        939      947       +8
Impacted Files Coverage Δ
reframe/core/runtime.py 89.69% <ø> (ø) ⬆️
unittests/resources/checks/frontend_checks.py 99.21% <ø> (ø) ⬆️
reframe/core/launchers/registry.py 85% <ø> (ø) ⬆️
unittests/resources/checks/bad/noentry.py 80% <ø> (ø) ⬆️
reframe/frontend/dependency.py 96.77% <ø> (ø) ⬆️
unittests/resources/settings.py 100% <ø> (ø) ⬆️
reframe/core/meta.py 100% <ø> (ø) ⬆️
reframe/core/exceptions.py 84.89% <ø> (+0.1%) ⬆️
reframe/core/systems.py 89.67% <ø> (ø) ⬆️
...ittests/resources/checks_unlisted/kbd_interrupt.py 100% <ø> (ø) ⬆️
... and 152 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dd6d377...828a26a. Read the comment docs.

Copy link
Contributor

@vkarak vkarak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that the unit test for the run-only tests case would pass even before the fix. The post_run is just commands to be inserted in the generated script. So, for sure, greetings.txt is going to be present in the stage directory regardless. The correct way to test is perhaps by manipulating the sourcesdir. You could set sourcesdir to None before run and then make sure that nothing was copied to the stage directory. Without the fix, this would fail, because it would run the hook after the copy to the stage directory.

@vkarak vkarak changed the title [bugfix] Fix CompileOnlyRegressionTest and RunOnlyRegressionTest hooks [bugfix] Execute setup hooks for CompileOnlyRegressionTest and execute the run hooks for the RunOnlyRegressionTest at the right place Mar 2, 2020
@vkarak vkarak changed the title [bugfix] Execute setup hooks for CompileOnlyRegressionTest and execute the run hooks for the RunOnlyRegressionTest at the right place [bugfix] Execute setup hooks for CompileOnlyRegressionTest and make the run hooks for the RunOnlyRegressionTest execute at the right place Mar 2, 2020
@pep8speaks
Copy link

pep8speaks commented Mar 3, 2020

Hello @ekouts, Thank you for updating!

Cheers! There are no PEP8 issues in this Pull Request!Do see the ReFrame Coding Style Guide

Comment last updated at 2020-03-10 10:03:24 UTC

Copy link
Contributor

@vkarak vkarak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@vkarak vkarak merged commit 7e90868 into reframe-hpc:master Mar 10, 2020
@ekouts ekouts deleted the bugfix/setup_hooks branch March 18, 2020 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Setup hooks don't work for CompileOnlyRegressionTest

4 participants