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

fix(earthly-flutter): Make sure earthly target fails when flutter tests fail #329

Merged

Conversation

dtscalac
Copy link
Contributor

@dtscalac dtscalac commented Oct 6, 2024

Description

Defers the error that can be raised after running flutter unit tests until the test_reports and coverage artifacts are saved, then raises an error to indicate that the target failed.

  • flutter test does not generate any test_reports therefore I'm removing the artifacts

Please confirm the following checks

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream module

@dtscalac dtscalac added ci/cd CI/CD Fixes or Improvements. review me PR is ready for review labels Oct 6, 2024
@dtscalac dtscalac self-assigned this Oct 6, 2024
Copy link
Collaborator

@stevenj stevenj left a comment

Choose a reason for hiding this comment

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

We do this kind of thing in the Rust builders too.
Be aware that it creates a side effect in the build.
Because the RUN melos run ... || touch fail will not actually fail, the result of that will be cached as a success by the build system.
Any terminal output it creates to help you diagnose the problem will then only be shown on the first run.

Even the two RUN after IF [ -f fail ] will also cache, only the last RUN will show as failed.
For that RUN, I would be inclined to make it:

RUN echo "msg"; \
         exit 1

So, at least the message is shown.
For the melos run test-report you might be able to use tee something like so:

RUN melos run test-report 2>&1 | tee melos-output.txt || touch fail

and then add a cat melos-output.txt to the part where you actually fail the build.
This would have the effect of always showing the output of the melos run on failure or success the first time, and on failure re-displaying the failure messages.

I have considered doing something like this in the Rust builders as well, but haven't implemented it yet.

@stevenj
Copy link
Collaborator

stevenj commented Oct 7, 2024

As a second comment, this is like this because of the coverage results and the way they are saved. We are looking for a better solution for those, and if we can do that without using a "SAVE ARTIFACT LOCALLY" we would no longer need to do this, and the RUN could be a script which does everything, and then fails or not. Which would make this code much less cumbersome.

@dtscalac
Copy link
Contributor Author

dtscalac commented Oct 7, 2024

We do this kind of thing in the Rust builders too. Be aware that it creates a side effect in the build. Because the RUN melos run ... || touch fail will not actually fail, the result of that will be cached as a success by the build system. Any terminal output it creates to help you diagnose the problem will then only be shown on the first run.

Even the two RUN after IF [ -f fail ] will also cache, only the last RUN will show as failed. For that RUN, I would be inclined to make it:

RUN echo "msg"; \
         exit 1

So, at least the message is shown. For the melos run test-report you might be able to use tee something like so:

RUN melos run test-report 2>&1 | tee melos-output.txt || touch fail

and then add a cat melos-output.txt to the part where you actually fail the build. This would have the effect of always showing the output of the melos run on failure or success the first time, and on failure re-displaying the failure messages.

I have considered doing something like this in the Rust builders as well, but haven't implemented it yet.

The tee command is difficult to use because it does not retain the exit code of the command that redirected output to it so the whole pipeline RUN melos run test-report 2>&1 | tee melos-output.txt || touch fail will never get to touch fail because tee always exits with exit code == 0.

I couldn't find any simple solutions to be able to use tee so I came up with idea of printing the whole output to a file, then replaying it from the file via cat. It's not perfect, we only get the logs once everything completes but it does work.

Earthly's TRY-FINALLY is still experimental, maybe in the future we could use it to make things less complex.

@dtscalac dtscalac requested a review from stevenj October 7, 2024 10:06
@dtscalac dtscalac merged commit 8135d23 into master Oct 7, 2024
65 checks passed
@dtscalac dtscalac deleted the fix/make-sure-earthly-target-fails-when-flutter-tests-fail branch October 7, 2024 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/cd CI/CD Fixes or Improvements. review me PR is ready for review
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

2 participants