status-im / status-desktop

Status Desktop client made in Nim & QML
https://status.app
Mozilla Public License 2.0
292 stars 78 forks source link

nim tests run in CI but do not report correctly #16545

Open jrainville opened 1 day ago

jrainville commented 1 day ago

Bug Report

Description

On PRs , we run Nim tests, but while they do run, they currently fail and still report as success.

For example, this test fails:

[2024-10-17T07:04:24.214Z] [Suite] collectibles model
[2024-10-17T07:04:24.214Z] options.nim(226)         get
[2024-10-17T07:04:24.214Z] 
[2024-10-17T07:04:24.214Z]     Unhandled exception: Can't obtain a value from a `none` [UnpackDefect]
[2024-10-17T07:04:24.214Z]   [FAILED] Collectible list set
[2024-10-17T07:04:24.214Z] options.nim(226)         get
[2024-10-17T07:04:24.214Z] 
[2024-10-17T07:04:24.214Z]     Unhandled exception: Can't obtain a value from a `none` [UnpackDefect]
[2024-10-17T07:04:24.214Z]   [FAILED] Collectible list update
[2024-10-17T07:04:24.214Z] Error: execution of an external program failed: '/home/jenkins/workspace/_linux_x86_64_tests-nim_PR-16541/bin/collectibles_model_test '

and yet, it shows as green:

[2024-10-17T07:04:37.297Z] + curl --silent -XPOST --data {
[2024-10-17T07:04:37.297Z]     "id": "#3",
[2024-10-17T07:04:37.297Z]     "commit": "fa9984c1",
[2024-10-17T07:04:37.297Z]     "success": true,

Steps to reproduce

just run a PR and see the Nim tests are always passing

Expected behavior

if a test fails, the report should be red and block the PR merging

Actual behavior

if a test fails, it shows green and the PR is mergable

jakubgs commented 1 day ago

There's nothing here that would prevent it from returning correct error code: https://github.com/status-im/status-desktop/blob/9332d1a2ed622b094367ec530908e77fd103a113/ci/Jenkinsfile.tests-nim#L64-L66 But this is suspect: https://github.com/status-im/status-desktop/blob/9332d1a2ed622b094367ec530908e77fd103a113/Makefile#L855-L857

jakubgs commented 1 day ago

And indeed, if you generate a list of commands with foreach like this only the exit code from the last one is registered by the Makefile:

FILES = what the fuck Makefile
.PHONY: main
main:
    set -x; \
    $(foreach file,$(FILES),ls $(file);)
 > make
set -x; \
ls what; ls the; ls fuck; ls Makefile;
+ ls what
ls: cannot access 'what': No such file or directory
+ ls the
ls: cannot access 'the': No such file or directory
+ ls fuck
ls: cannot access 'fuck': No such file or directory
+ ls Makefile
Makefile
jakubgs commented 1 day ago

We could prevent this by combining the commands using &&:

 > make
set -x \
&& ls what && ls the && ls fuck && ls Makefile
+ ls what
ls: cannot access 'what': No such file or directory
make: *** [Makefile:4: main] Error 2

But then only the first call actually finishes, and the rest is never executed. Not sure if that's desirable.

jrainville commented 1 day ago

But then only the first call actually finishes, and the rest is never executed. Not sure if that's desirable.

It's better than nothing, but it would be better if all of them executed still I think

jakubgs commented 1 day ago

Then the correct solution would involve generating targets dynamically.

jakubgs commented 1 day ago

Fix: