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

Catch unit test failures in make test #5635

Merged
merged 3 commits into from
Jan 30, 2024
Merged

Catch unit test failures in make test #5635

merged 3 commits into from
Jan 30, 2024

Conversation

Groxx
Copy link
Contributor

@Groxx Groxx commented Jan 29, 2024

Surprisingly, this has not worked since tests were introduced to the makefile in 2017.
We've apparently been relying on CI's grepping for go test failure lines or something.

Rather trivially, without -o pipefail this only returns error messages from tee:

> false | tee -a file.log
> echo $?
0

Which means this also does not return exit 1:

for ... do
  go test whatever | tee -a file.log;
done

Which means you can fail a test and the tests keep going, and it exits 0:

> make test
...
FAIL	github.com/uber/cadence/common/util	0.245s
FAIL
ok  	github.com/uber/cadence/common/locks	0.252s	coverage: 93.3% of statements
...

> echo $?
0

Which kinda stinks for local development. Failing tests can be hard to spot in the massive output of V=1, or dozens of lines off-screen if a lot of successful packages ran afterward.

Now this will continue running tests, but will clearly fail with a list of all failed packages:

> make test
...
FAIL    github.com/uber/cadence/common/util     0.245s
FAIL
ok      github.com/uber/cadence/common/locks    0.252s  coverage: 93.3% of statements
...
Failed packages:  ./common/util
make: *** [test] Error 1

> echo $?
1

Which is roughly how go test ./... works: it fails but keeps going, and does an exit 1 if there were failures along the way (though it does not show the list of packages).


This also removes bins as a prerequisite for tests, as:

  • it's slow and usually not necessary
  • we now have make build which is MUCH more complete
  • CI does this explicitly anyway, no need to do it more than once.

Surprisingly, this has not worked since tests were introduced to the makefile _in 2017_.
We've apparently been relying on CI's grepping for go test failure lines or something.

Rather trivially, without `-o pipefail` this only returns error messages from `tee`:
```bash
> false | tee -a file.log
> echo $?
0
```
Which means this also does not return `exit 1`:
```bash
for ... do
  go test whatever | tee -a file.log;
done
```
Which means you can fail a test and the tests keep going, and it exits 0:
```bash
> make test
...
FAIL	github.com/uber/cadence/common/util	0.245s
FAIL
ok  	github.com/uber/cadence/common/locks	0.252s	coverage: 93.3% of statements
...

> echo $?
0
```
Which kinda stinks for local development.  Failing tests can be hard to spot in the massive output of `V=1`, or dozens of lines off-screen if a lot of successful packages ran afterward.

Now this will continue running tests, but will clearly fail with a list of all failed packages:
```bash
> make test
...
FAIL    github.com/uber/cadence/common/util     0.245s
FAIL
ok      github.com/uber/cadence/common/locks    0.252s  coverage: 93.3% of statements
...
Failed packages:  ./common/util
make: *** [test] Error 1

> echo $?
1
```
Which is roughly how `go test ./...` works: it fails but keeps going, and does an `exit 1` if there were failures along the way (though it does not show the list of packages).
@coveralls
Copy link

coveralls commented Jan 29, 2024

Pull Request Test Coverage Report for Build 018d5b55-f370-4608-bd47-098425223d02

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 94 unchanged lines in 12 files lost coverage.
  • Overall coverage decreased (-0.02%) to 62.719%

Files with Coverage Reduction New Missed Lines %
common/persistence/historyManager.go 2 66.67%
common/task/weighted_round_robin_task_scheduler.go 2 89.05%
service/history/execution/mutable_state_builder.go 2 68.64%
service/history/task/transfer_active_task_executor.go 2 72.22%
service/matching/taskReader.go 2 84.88%
common/persistence/statsComputer.go 3 94.29%
service/history/task/fetcher.go 4 86.08%
common/persistence/nosql/nosqlplugin/cassandra/workflow.go 6 59.1%
service/history/task/transfer_standby_task_executor.go 10 85.77%
common/persistence/sql/workflowStateMaps.go 11 81.25%
Totals Coverage Status
Change from base Build 018d51d8-dff5-438a-9f3f-3b343e5e5478: -0.02%
Covered Lines: 92277
Relevant Lines: 147128

💛 - Coveralls

Makefile Outdated Show resolved Hide resolved
@@ -597,10 +608,10 @@ cover_profile:
$Q for dir in $(PKG_TEST_DIRS); do \
mkdir -p $(BUILD)/"$$dir"; \
go test "$$dir" $(TEST_ARG) -coverprofile=$(BUILD)/"$$dir"/coverage.out || exit 1; \
cat $(BUILD)/"$$dir"/coverage.out | grep -v "^mode: \w\+" >> $(UNIT_COVER_FILE); \
(cat $(BUILD)/"$$dir"/coverage.out | grep -v "^mode: \w\+" >> $(UNIT_COVER_FILE)) || true; \
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the grep filter for? the fact that multiline comments are hard would make me lean to towards throwing into a bash script

Copy link
Contributor Author

@Groxx Groxx Jan 30, 2024

Choose a reason for hiding this comment

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

Historically, it was because you can just concat multiple coverage files to have go transparently compute the combined coverage... except for those mode lines, which would cause it to error.

Nowadays I know there are better options in a few ways, but I haven't dug into them in detail yet.


Re script file: yeah, these loops are borderline or slightly past I think. But it is hopefully temporary and will also-hopefully be simpler after a modernization pass, so I'm kinda reluctant to do more than the minimum / putting it in here keeps the badness visible until it's replaced / etc :/

I could be convinced to do otherwise, just didn't feel like it was worth it at the moment.

Copy link
Contributor

@davidporter-id-au davidporter-id-au left a comment

Choose a reason for hiding this comment

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

lgtm with nit about logs

@Groxx Groxx merged commit ae5e97b into master Jan 30, 2024
16 checks passed
@Groxx Groxx deleted the fixmake branch January 30, 2024 17:40
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