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

test: Add json marshal/unmarshal for test traces #1314

Merged
merged 32 commits into from
Sep 25, 2023
Merged

Conversation

p-offtermatt
Copy link
Contributor

@p-offtermatt p-offtermatt commented Sep 19, 2023

Description

Closes: Part of #1265

The PR introduces a trace format that is just a JSON serialization of a step slice. This means it makes use of all the actions we already have.

To review this...

  • check that in the _rapid_test.go files, the structs are built exhaustively, i.e. I didn't forget to add any fields that aren't populated now
  • check that the switch-cases are exhaustive, i.e. no actions/proposals that exist are missed by the switch-case in json_utils.go
  • take a look at the tracehandlers_testdata folder and see the json format, check if you see any obvious defects. viewing these is much nicer after either formatting them, putting them into a json viewer, ...
  • check out the meat of the changes: the actual ser/de is done in tests/e2e/json_parser.go, tests/e2e/json_writer.go and tests/e2e/json_utils.go

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • Included the correct type prefix in the PR title
  • Targeted the correct branch (see PR Targeting)
  • Provided a link to the relevant issue or specification
  • Reviewed "Files changed" and left comments if necessary
  • Confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • Confirmed the correct type prefix in the PR title
  • Confirmed all author checklist items have been addressed
  • Confirmed that this PR does not change production code

@github-actions github-actions bot added the C:Testing Assigned automatically by the PR labeler label Sep 19, 2023
@p-offtermatt p-offtermatt linked an issue Sep 19, 2023 that may be closed by this pull request
@github-actions github-actions bot added C:Build Assigned automatically by the PR labeler C:CI Assigned automatically by the PR labeler labels Sep 19, 2023
@github-actions github-actions bot removed C:Build Assigned automatically by the PR labeler C:CI Assigned automatically by the PR labeler labels Sep 19, 2023
Base automatically changed from ph/refactor-e2e-structs to main September 19, 2023 18:39
@p-offtermatt p-offtermatt marked this pull request as ready for review September 20, 2023 07:55
@p-offtermatt p-offtermatt requested a review from a team as a code owner September 20, 2023 07:55
@p-offtermatt p-offtermatt changed the title Add json marshal/unmarshal for test traces test: Add json marshal/unmarshal for test traces Sep 20, 2023
tests/e2e/json_parser.go Outdated Show resolved Hide resolved
@insumity
Copy link
Contributor

Assume we add a new Action in the future. Based on this PR, I would assume we would need to modify a bunch of files to include this Action? What would be the process for that? Would it make sense to have a README.md file under tests/e2e to describe such a process (e.g., what files need to be changed, etc.)?

@p-offtermatt
Copy link
Contributor Author

p-offtermatt commented Sep 20, 2023

Assume we add a new Action in the future. Based on this PR, I would assume we would need to modify a bunch of files to include this Action? What would be the process for that? Would it make sense to have a README.md file under tests/e2e to describe such a process (e.g., what files need to be changed, etc.)?

yeah exactly. I'll draft up a README like that. I'll also put an issue in the backlog to look into setting up a static analyzer that could flag this. From all I can tell, the current solution is unfortunately the best I could find, so supporting it to be more smooth would probably be desirable.

EDIT: Added e837330

Copy link
Contributor

@insumity insumity left a comment

Choose a reason for hiding this comment

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

Great work! Thanks for the changes.

tests/e2e/json_utils.go Outdated Show resolved Hide resolved
tests/e2e/json_utils.go Outdated Show resolved Hide resolved
@sainoe
Copy link
Contributor

sainoe commented Sep 21, 2023

Great work @p-offtermatt! See my comments.
Note that I haven't gone through everything yet, but I will cover the rest ASAP.

tests/e2e/json_utils.go Outdated Show resolved Hide resolved
Copy link
Contributor

@shaspitz shaspitz left a comment

Choose a reason for hiding this comment

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

Great work! Added some comments w.r.t boilerplate and maintenance moving forward

tests/e2e/action_rapid_test.go Show resolved Hide resolved
tests/e2e/action_rapid_test.go Show resolved Hide resolved
tests/e2e/json_utils.go Show resolved Hide resolved
tests/e2e/state_rapid_test.go Show resolved Hide resolved
tests/e2e/trace_handlers_test.go Show resolved Hide resolved
tests/e2e/json_utils.go Outdated Show resolved Hide resolved
@sainoe sainoe self-requested a review September 22, 2023 07:37
@p-offtermatt p-offtermatt merged commit 3d4627b into main Sep 25, 2023
13 checks passed
@p-offtermatt p-offtermatt deleted the ph/json-marshal branch September 25, 2023 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:Testing Assigned automatically by the PR labeler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modify the e2e framework to take traces as an input
4 participants