Add support for cleanup steps #163

vjt · 2023-05-17T15:22:12Z

This implements feature request #143.

Missing:

handling the pause event while in cleanup steps - DONE
handling errors in cleanup steps - DONE by ignoring them
unit tests - DONE
integration tests - DONE

mimir-d · 2023-05-17T21:57:57Z

youve a broken test;
what should i understand from the "missing" section above? option 1. you'd like this PR merged and then you add others separately (in which case, i need to check if this one is functionally complete for what it does). or option 2. this a WIP PR and you'll add more commits to it

vjt · 2023-05-18T08:11:54Z

youve a broken test;

working on fixing that

what should i understand from the "missing" section above? option 1. you'd like this PR merged and then you add others separately (in which case, i need to check if this one is functionally complete for what it does). or option 2. this a WIP PR and you'll add more commits to it

sorry, I wrote that in a hurry. this is a WIP PR and I'll add more commits to it. I'd like your review on what it has been done so far, whether it is reasonable or not, and your insights on the missing items :-)

Thank you!

Signed-off-by: Marcello Barnaba <mbarnaba@meta.com>

This allows skipping tests that rely on MongoDB, if for instance you are running this environment on an M1 Mac with QEMU via UTM that does not yet support the AVX machine instructions that MongoDB currently requires. Signed-off-by: Marcello Barnaba <mbarnaba@meta.com>

Signed-off-by: Marcello Barnaba <mbarnaba@meta.com>

The cleanup steps are embedded within a TestDescriptor. They are retrieved via an optional separated test fetcher called "CleanupFetcherName" and with its parameters in the "CleanupFetcherTestParameters" property. The TestFetcherBundle structure in pkg/test/fetcher.go is changed adding the cleanup steps alongside the existing test steps. The pluginregistry/bundles.go change checks whether the cleanup fetcher is set and whether its parameters are valid. If that is the case, the cleanup fetcher and its parameters are set in the TestFetcherBundle structure. Then, the TestStepsDescriptors structure in pkg/test/step.go is changed adding again the cleanup name and the resolved cleanup steps alongside the existing test steps. The change in pkg/jobmanager/steps.go checks whether a cleanup fetcher is set and it invokes it passing its parameter to obtain concrete cleanup steps descriptors, that are saved in the TestStepsDescriptors structure. The concrete cleanup steps are stored in the Test object, used at runtime, alongside the existing test steps, and modeled as a list of TestStepBundle structures. The change in pkg/jobmanager/job.go checks whether cleanup steps are defined in the TestStepsDescriptors when building the Test object, and if yes it creates the step bundles for the cleanup steps. The change in pkg/jobmanager/bundles.go massages the existing methods to allow passing a list of test steps instead of a TestStepsDescriptor, so to be able to re-use the methods for both TestSteps and CleanupSteps. Yay for layering :-) Signed-off-by: Marcello Barnaba <mbarnaba@meta.com>

As this is required in the next changeset where each step gets a separate event emitter. Signed-off-by: Marcello Barnaba <mbarnaba@meta.com>

codecov-commenter · 2023-05-18T14:00:22Z

Codecov Report

Patch coverage: 81.06% and project coverage change: +0.13 🎉

Comparison is base (4d6d981) 63.17% compared to head (165eac1) 63.30%.

❗ Current head 165eac1 differs from pull request most recent head 23654f4. Consider uploading reports for the commit 23654f4 to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #163      +/-   ##
===========================================
+ Coverage    63.17%   63.30%   +0.13%     
===========================================
  Files          165      165              
  Lines        10473    10587     +114     
===========================================
+ Hits          6616     6702      +86     
- Misses        3120     3140      +20     
- Partials       737      745       +8

Flag	Coverage Δ
e2e	`48.51% <59.48%> (+0.05%)`	⬆️
integration	`54.49% <76.51%> (+0.15%)`	⬆️
unittests	`49.54% <58.46%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/job/events.go	`86.66% <ø> (ø)`
pkg/test/step.go	`77.77% <ø> (ø)`
pkg/test/test.go	`33.33% <ø> (ø)`
plugins/testfetchers/uri/uri.go	`3.57% <0.00%> (ø)`
tests/integ/jobmanager/jobdescriptors.go	`71.42% <ø> (ø)`
plugins/testfetchers/literal/literal.go	`62.50% <50.00%> (ø)`
pkg/runner/test_runner.go	`85.57% <61.90%> (-1.55%)`	⬇️
pkg/jobmanager/job.go	`63.82% <72.50%> (+3.33%)`	⬆️
pkg/pluginregistry/bundles.go	`63.85% <75.00%> (+0.99%)`	⬆️
pkg/runner/job_runner.go	`79.84% <75.90%> (-1.00%)`	⬇️
... and 10 more

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

vjt

Adding some comments where I think we still need work

pkg/runner/job_runner.go

tests/integ/jobmanager/common.go

pkg/jobmanager/bundles.go

pkg/jobmanager/job.go

pkg/pluginregistry/bundles.go

pkg/runner/test_runner.go

pkg/test/step.go

tests/integ/jobmanager/common.go

mimir-d · 2023-05-25T22:26:39Z

please also change this to merge into develop instead of main

This change implements the core of cleanup steps runs after test steps have completed. It is done in a way that steps variables can be shared between test steps and cleanup steps. Signed-off-by: Marcello Barnaba <mbarnaba@meta.com>

mimir-d

not many changes, but one of the tests is failing

plugins/storage/rdbms/events.go

pkg/runner/job_runner_test.go

Signed-off-by: Marcello Barnaba <mbarnaba@meta.com>

As the cleanup steps are strictly related to the test they refer to, remove the requirement to be in a named test. Signed-off-by: Marcello Barnaba <mbarnaba@meta.com>

vjt · 2023-05-27T15:06:03Z

Tests finally pass! 0f7af7c was particularly painful, because I was thinking to make a change affecting only test code, but in reality the code that fetches from storages is the same across tests and prod. Anyway, the change looks reasonable to me.

Cheers

mimir-d · 2023-05-27T20:02:03Z

thanks for working on this

vjt force-pushed the feature/143 branch 12 times, most recently from d08bd5f to 6d604c5 Compare May 18, 2023 13:01

Squelch format-related warnings

dc4059d

Signed-off-by: Marcello Barnaba <mbarnaba@meta.com>

vjt force-pushed the feature/143 branch from 6d604c5 to 4125b76 Compare May 18, 2023 13:41

vjt added 5 commits May 18, 2023 15:46

Remove unreachable code

b0a3a53

Signed-off-by: Marcello Barnaba <mbarnaba@meta.com>

Stub out test objects creation in a separate func

03b5d6d

Signed-off-by: Marcello Barnaba <mbarnaba@meta.com>

Cleanup steps: Ensure different labels vs. tests

9e60171

As this is required in the next changeset where each step gets a separate event emitter. Signed-off-by: Marcello Barnaba <mbarnaba@meta.com>

vjt force-pushed the feature/143 branch from 4125b76 to 499b26e Compare May 18, 2023 13:46

vjt commented May 19, 2023

View reviewed changes

pkg/runner/job_runner.go Outdated Show resolved Hide resolved

pkg/runner/job_runner.go Outdated Show resolved Hide resolved

tests/integ/jobmanager/common.go Outdated Show resolved Hide resolved

mimir-d requested changes May 23, 2023

View reviewed changes

vjt force-pushed the feature/143 branch 4 times, most recently from f254b7d to 43ac3c3 Compare May 26, 2023 12:22

Cleanup steps: run them after test steps

6ca60a5

This change implements the core of cleanup steps runs after test steps have completed. It is done in a way that steps variables can be shared between test steps and cleanup steps. Signed-off-by: Marcello Barnaba <mbarnaba@meta.com>

vjt force-pushed the feature/143 branch from 43ac3c3 to 6ca60a5 Compare May 26, 2023 13:21

vjt changed the base branch from main to develop May 26, 2023 14:02

vjt force-pushed the feature/143 branch from 283a7b9 to 0456c76 Compare May 26, 2023 14:18

mimir-d requested changes May 26, 2023

View reviewed changes

plugins/storage/rdbms/events.go Outdated Show resolved Hide resolved

pkg/runner/job_runner_test.go Outdated Show resolved Hide resolved

vjt force-pushed the feature/143 branch 4 times, most recently from bc11a09 to 847392f Compare May 27, 2023 14:30

vjt added 2 commits May 27, 2023 16:49

Tests: allow retrieving events for more than one test

0f7af7c

Signed-off-by: Marcello Barnaba <mbarnaba@meta.com>

Cleanup steps: do not require a cleanup test name

23654f4

As the cleanup steps are strictly related to the test they refer to, remove the requirement to be in a named test. Signed-off-by: Marcello Barnaba <mbarnaba@meta.com>

vjt force-pushed the feature/143 branch from 847392f to 23654f4 Compare May 27, 2023 14:49

mimir-d approved these changes May 27, 2023

View reviewed changes

mimir-d merged commit 57569d4 into linuxboot:develop May 27, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for cleanup steps #163

Add support for cleanup steps #163

vjt commented May 17, 2023 •

edited

Loading

mimir-d commented May 17, 2023

vjt commented May 18, 2023

codecov-commenter commented May 18, 2023 •

edited

Loading

vjt left a comment

mimir-d commented May 25, 2023

mimir-d left a comment

vjt commented May 27, 2023

mimir-d commented May 27, 2023

Add support for cleanup steps #163

Add support for cleanup steps #163

Conversation

vjt commented May 17, 2023 • edited Loading

mimir-d commented May 17, 2023

vjt commented May 18, 2023

codecov-commenter commented May 18, 2023 • edited Loading

Codecov Report

vjt left a comment

Choose a reason for hiding this comment

mimir-d commented May 25, 2023

mimir-d left a comment

Choose a reason for hiding this comment

vjt commented May 27, 2023

mimir-d commented May 27, 2023

vjt commented May 17, 2023 •

edited

Loading

codecov-commenter commented May 18, 2023 •

edited

Loading