Ensure we still rerun tests when there are warnings #235
Conversation
|
Thanks for working on this fix! Sorry it's taken me a bit of time to review. I think what you have looks like a reasonable way to solve the problem. I'll take a closer look and try to fix the test failure in the next few days. |
|
I think the test failures are because the |
dnephin
force-pushed
the
asb/ignore-warnings
branch
from
512bb97 to
283d44e
Compare
Use an explicit format so that the env var set in CI doesn't change the output And update the golden file to match.
internal/text/testing.go
Outdated
| var trailingElapsedTimePattern = regexp.MustCompile(`\s*\(\d+(\.\d+)?m?s\)\s*$`) | ||
|
|
||
| func OpRemoveTestElapsedTime(line string) string { | ||
| if i := strings.Index(line, " (0."); i > 0 && i+8 == len(line) { | ||
| return line[:i] | ||
| } | ||
| return line | ||
| return trailingElapsedTimePattern.ReplaceAllString(line, "") |
There was a problem hiding this comment.
I don't think I see why this change was made. I generally try to avoid regex. Reverting this still has the tests passing.
I'm going to revert this part of the change for now. We can always re-introduce it later if it fixed something that I'm missing.
There was a problem hiding this comment.
The time was being reported in ms rather than seconds so it didn't start with 0. Sounds like changing GOTESTSUM_FORMAT will fix that.
dnephin
force-pushed
the
asb/ignore-warnings
branch
from
283d44e to
bba51ac
Compare
Proposal for skipping over warnings.
Elapsed times in golden files are causing this to fail. It's also a pretty heavy-weight test for such a small change so maybe we should test this some other way.
Fixes #232