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

tests: fix flaky test for hooks undo #5268

Merged
merged 1 commit into from
Jun 6, 2018

Conversation

stolowski
Copy link
Contributor

@stolowski stolowski commented Jun 5, 2018

Fix improper (and unintended) use of task/change variables initialized in test setup. With this PR the undo test creates own task/change variables, local to the test. The problem is that TestSetup creates a configure task and adds it to s.change, the tasks in the undo test itself don't wait for it so the order of execution is random with regard to configure, and this occasionally yelds unexpected status of entire change and the hook task in question when everything is undone.

@stolowski stolowski added ⚠ Critical High-priority stuff (e.g. to fix master) Simple 😃 A small PR which can be reviewed quickly labels Jun 5, 2018
Copy link
Collaborator

@zyga zyga left a comment

Choose a reason for hiding this comment

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

Looks ok but I trust it more than understand it

@codecov-io
Copy link

codecov-io commented Jun 5, 2018

Codecov Report

Merging #5268 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5268      +/-   ##
==========================================
- Coverage   78.85%   78.84%   -0.02%     
==========================================
  Files         498      498              
  Lines       37488    37488              
==========================================
- Hits        29562    29557       -5     
- Misses       5528     5531       +3     
- Partials     2398     2400       +2
Impacted Files Coverage Δ
overlord/hookstate/hookmgr.go 74.27% <0%> (-2.43%) ⬇️

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 b6b0bf3...bd859f7. Read the comment docs.

@zyga zyga removed the Simple 😃 A small PR which can be reviewed quickly label Jun 6, 2018
@zyga
Copy link
Collaborator

zyga commented Jun 6, 2018

I took the simple out as it was short but definitely required some insight into what is going on in the test.

@stolowski stolowski merged commit b8e227c into snapcore:master Jun 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
⚠ Critical High-priority stuff (e.g. to fix master)
Projects
None yet
4 participants