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

Reset stonewall timer to make it work again with running all phases #120

Merged
merged 1 commit into from
Dec 19, 2018

Conversation

JulianKunkel
Copy link
Collaborator

Second part of bugfix for #119 allowing to use all phases in one execution.

…n one execution; i.e. not using multiple runs specifying: -C, -r
@glennklockwood glennklockwood merged commit 1b28a64 into master Dec 19, 2018
@glennklockwood
Copy link
Contributor

Thanks for catching (and fixing) this. Is this something that we can add to the test suite to catch any similar regressions in correctness around this functionality?

@JulianKunkel
Copy link
Collaborator Author

JulianKunkel commented Dec 19, 2018 via email

@johnbent
Copy link
Collaborator

johnbent commented Dec 19, 2018 via email

@glennklockwood
Copy link
Contributor

A unit test could probably catch this in principle, but mdtest is not factored very well for unit testing. We also never decided on a unit test framework since the only proposed solution (#38) required pulling in C++.

Since this error was just a number coming out wrong at the end of mdtest, I guess it's unclear how a functional test would catch this too. Unit tests are the right answer, but it'll be a lot of effort to get that sorted out I think.

@JulianKunkel JulianKunkel deleted the fix-119 branch December 22, 2018 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants