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

Add e2e test adding metadata to skeleton tracing #8113

Merged
merged 3 commits into from
Oct 9, 2024
Merged

Conversation

frcroth
Copy link
Member

@frcroth frcroth commented Oct 7, 2024

Added an e2e test that contains an update action that updates metadata. Some notes:

  • This could also be included into the "Send complex update actions and compare resulting tracing" test if we want to keep duplication / number of tests lower
  • This does not include tests for adding metadata to volume annotations. It seems there arent any tests for updating volume tracings?

Issues:

@frcroth frcroth requested a review from fm3 October 7, 2024 12:54
@frcroth frcroth self-assigned this Oct 7, 2024
Copy link
Member

@fm3 fm3 left a comment

Choose a reason for hiding this comment

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

Looking good, thanks!

Could you have a look at how long this new test is running? If it is slow, maybe it can be combined with another test that already creates trees? That way I would hope the CI time does not increase.

I am not super familiar with the existing tests. If there is really no test for updating volume tracings, maybe that would be a good new issue as well. Though if one of the update routes is tested, that already covers a lot of the code used by both skeleton and volume, so that wouldn’t have super high priority

@frcroth
Copy link
Member Author

frcroth commented Oct 9, 2024

This new test does not seem to really affect CI time. For me locally, it takes about 0.2 s.
It can definitely be combined with an existing test, if that is desired.

Copy link
Member

@fm3 fm3 left a comment

Choose a reason for hiding this comment

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

ok for 0.2s I’d say we can leave it as is for now :)

@frcroth frcroth merged commit fb4372a into master Oct 9, 2024
2 checks passed
@frcroth frcroth deleted the e2e-metadata branch October 9, 2024 11:22
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.

e2e tests for segment + tree metadata
2 participants