Skip to content

Fix test.sh#54

Merged
danielhollas merged 2 commits into
masterfrom
fix-tests-again
Jan 13, 2021
Merged

Fix test.sh#54
danielhollas merged 2 commits into
masterfrom
fix-tests-again

Conversation

@danielhollas

@danielhollas danielhollas commented Jan 12, 2021

Copy link
Copy Markdown
Contributor

Unfortunately, when I refactored test.sh in #50 to make it more resilient, I broke it. :-(
Specifically, it was always returning 0, even if some tests failed.
In turn, Github CI was not failing when tests where failing. (as seen e.g. here
And this is also a reason why the coverage changes in #49 were so wonky...I should have taken a hint.

Apart from fixing the test.sh, I'm also fixing the failing tests here.

  1. Use of unitialized variable equant in analysis.F90 caused any non-Path-Integral test to fail when compiled with
    floating-point checks (uncovered in PR Move source to src/ #50).
    Fortunately, this bug did not really affect anything because we don't currently print equant anywhere.

  2. Non-significant numerical difference in the HARMON test,
    which started showing up after a fix to numdiff.py in 4620895 in MAJOR fix to tests! #51.

A lesson here is, never touch test.sh again without opening a dedicated PR that can be carefully reviewed.
The error was introduced in PR #50 which unrelated to tests.

Unfortunately, when I refactored test.sh to make
it more resilient, I broke it. :-(
Specifically, it was always returning 0,
even if some tests failed.
In turn, Github CI was not failing when tests where failing.
1. Use of unitialized variable `equant` in analysis.F90
   caused any non-PI test to fail when compiled with
   floating-point checks (uncovered in PR #50).
   Fortunately, this bug did not really affect anything
   because we don't currently print equant anywhere.

2. Non-significant numerical difference in the HARMON test,
   which started showing up after a fix to numdiff.py in 4620895 in #51.
@codecov

codecov Bot commented Jan 12, 2021

Copy link
Copy Markdown

Codecov Report

Merging #54 (f889ef8) into master (c67d290) will decrease coverage by 0.07%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #54      +/-   ##
==========================================
- Coverage   59.78%   59.71%   -0.08%     
==========================================
  Files          36       36              
  Lines        5575     5575              
==========================================
- Hits         3333     3329       -4     
- Misses       2242     2246       +4     
Impacted Files Coverage Δ
src/abin.F90 74.64% <ø> (ø)
src/init.F90 60.37% <0.00%> (-0.58%) ⬇️

@danielhollas danielhollas marked this pull request as ready for review January 12, 2021 22:42
@danielhollas

Copy link
Copy Markdown
Contributor Author

@suchanj can you take a look?

The coverage change in init.F90, which is not modified here at all, are because the failing test were triggering error code paths that we're currently not testing, see: https://app.codecov.io/gh/PHOTOX/ABIN/compare/54/changes#D1L1095

@danielhollas danielhollas requested a review from suchanj January 12, 2021 23:00
@suchanj

suchanj commented Jan 13, 2021

Copy link
Copy Markdown
Contributor

It all makes sense now, good job @danielhollas unraveling this mystery. I agree with dedicated PRs for test.sh. I wish this could be enforced by some security settings, but minding the red flags and co-reviewing should do.

@danielhollas danielhollas merged commit 142515a into master Jan 13, 2021
@danielhollas danielhollas deleted the fix-tests-again branch January 13, 2021 12:50
@danielhollas

Copy link
Copy Markdown
Contributor Author

Yeah, there were multiple red flags that I didn't paid closer attention to, this one is on me.
I actually did try to make the test.sh more robust against programming errors by using set -euo pipefail, but it clearly cannot catch all errors. Weird Codecov report is actually a good sign. Especially if it is reporting coverage changes in files that were not touched in a given PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants