Skip to content

Improve helm test output; and provide the means (even if disabled due to flaky tests) for parallel helm test executions.#3040

Merged
jschaul merged 24 commits intodevelopfrom
make-use-of-parallelism-more
Feb 2, 2023
Merged

Improve helm test output; and provide the means (even if disabled due to flaky tests) for parallel helm test executions.#3040
jschaul merged 24 commits intodevelopfrom
make-use-of-parallelism-more

Conversation

@jschaul
Copy link
Member

@jschaul jschaul commented Jan 30, 2023

  1. Allow running helm tests in parallel if desired, using HELM_PARALLELISM=6 (disabled for now until we have fixed some flaky tests which fail more often when tests run in parallel)

  2. rework integration test output: logs from test runs will only show if there are any failed tests. Also, the bottom of the output will have a summary of what failed and what didn't; as well as only the failed test lines with a context of +- 10 lines. This should hopefully make it easier to see what went wrong: just scroll to the bottom.

The summary looks like this:

=== tail cargohold: ===

All 21 tests passed (8.45s)
=== tail gundeck: ===

All 33 tests passed (56.60s)
=== tail federator: ===
Finished in 0.6576 seconds
9 examples, 0 failures
=== tail spar: ===
Finished in 397.2779 seconds
553 examples, 0 failures, 65 pending
=== tail brig: ===

2 out of 449 tests failed (123.07s)
=== tail galley: ===

1 out of 414 tests failed (136.33s)
cargohold-integration passed ✅.
gundeck-integration passed ✅.
federator-integration passed ✅.
spar-integration passed ✅.
brig-integration FAILED ❌. pfff...
galley-integration FAILED ❌. pfff...
Tests failed.

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@jschaul jschaul requested a review from akshaymankar January 30, 2023 16:56
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Jan 30, 2023
@jschaul jschaul requested a review from smatting January 30, 2023 18:10
@jschaul

This comment was marked as resolved.

@smatting
Copy link
Contributor

The relevant log section seems to be missing the line of the failing test.
In this case this line is missing

POST /register - can add team members above fanout limit when whitelisting is enabled:

looks like it got overwritten by

galley-integration: Thread killed by timeout manager

https://concourse.ops.zinfra.io/teams/main/pipelines/wire-server-pr/jobs/integration-tests/builds/1461#L639a4d00:4361

@jschaul jschaul force-pushed the make-use-of-parallelism-more branch from 1598bd7 to 0ef5a5f Compare January 31, 2023 16:39
@jschaul
Copy link
Member Author

jschaul commented Jan 31, 2023

The relevant log section seems to be missing the line of the failing test. In this case this line is missing

POST /register - can add team members above fanout limit when whitelisting is enabled:

looks like it got overwritten by

galley-integration: Thread killed by timeout manager

https://concourse.ops.zinfra.io/teams/main/pipelines/wire-server-pr/jobs/integration-tests/builds/1461#L639a4d00:4361

Good catch. It happens when there is a line like

some test name:                      {"level": "Info", ....

where the test name is filtered out as it's on the same line as some noisy log lines. I added some sed magic to deal with these cases in the last commit.

@jschaul jschaul force-pushed the make-use-of-parallelism-more branch from 34cac72 to b5231c3 Compare February 1, 2023 16:31
@jschaul jschaul mentioned this pull request Feb 1, 2023
2 tasks
Comment on lines +48 to +50
Copy link
Member

Choose a reason for hiding this comment

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

I think this hopes that the red colour is reset during the next 10 lines and no other colour is set at that point, which might not be true and mess up rest of the output. I think we should explicitly print a colour reset code after this rg to ensure that doesn't happen.

Copy link
Member Author

Choose a reason for hiding this comment

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

this is kind of the end of the ouput for the integration tests. I've not observde any problems with colour so far. But happy to add something. Would a echo -e "\033[0m" line after the rg line do the trick?

Copy link
Member

Choose a reason for hiding this comment

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

Would a echo -e "\033[0m" line after the rg line do the trick?

I think that should work.

Copy link
Member

Choose a reason for hiding this comment

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

Will this directory need to be cleaned up to run the test next time?

Copy link
Member Author

Choose a reason for hiding this comment

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

this is just a marker file to avoid gnu parallel complaining about wanting to be cited in academic papers. No need for cleanup if this runs locally.

(the logs-brig and stat-brig files so far remain on disk if you run the integration test locally)

Copy link
Member Author

Choose a reason for hiding this comment

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

(update: the script will now clean up these local temporary files by default)

@jschaul jschaul force-pushed the make-use-of-parallelism-more branch from 578b987 to 57178ff Compare February 2, 2023 10:28
@akshaymankar
Copy link
Member

Maybe I should've pointed it out in #3049, but now we don't need the forked helm, maybe we can also get rid of that?

@jschaul
Copy link
Member Author

jschaul commented Feb 2, 2023

Maybe I should've pointed it out in #3049, but now we don't need the forked helm, maybe we can also get rid of that?

I already thought of using the fork to get another patch in, this one: helm/helm#11766

But perhaps we can also wait until that is merged (if it ever happens).

@jschaul jschaul changed the title Run helm tests in parallel Improve helm test output; and provide the means (even if disabled due to flaky tests) for parallel helm test executions. Feb 2, 2023
Copy link
Contributor

@smatting smatting Feb 2, 2023

Choose a reason for hiding this comment

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

No need to attribute to me here ;)

@jschaul jschaul force-pushed the make-use-of-parallelism-more branch from 0b434e0 to 5bd9c0a Compare February 2, 2023 16:17
@jschaul jschaul merged commit 7d1504b into develop Feb 2, 2023
@jschaul jschaul deleted the make-use-of-parallelism-more branch February 2, 2023 16:18
@migueleliasweb
Copy link

I already thought of using the fork to get another patch in, this one: helm/helm#11766

But perhaps we can also wait until that is merged (if it ever happens).

I really hope it does eventually get merged! 🙏

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

Labels

ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

Comments