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

features: Implement JSON trace format #878

Conversation

p-offtermatt
Copy link
Contributor

@p-offtermatt p-offtermatt commented Apr 24, 2023

HOW-TO-READ commit by commit:

81ec379 adds first version of the parser and writer with test files
8c42810 is a large commit which exports action and state wherever they are mentioned. just a chore. all changes should be the words action to Action and state to State
ec6a6bf is also a large commit. It exports all fields of all action types. All changes should be capitalizing words
f1c3c0f is also a chore: it maps strings of action types to their action structs
e6753ff refactors the first version of the parser and writer, and contains much of the final logic
85f5d15 adds some examples of what the trace format looks like

Description

Add a writer and reader for a simple JSON file format which is marshalled directly from
the Step objects in our code.

There are some chores which involve changing lots of fields to be exported (so that they can be properly Unmarshalled/Marshalled).

Closes: #875

Add a parser/writer to input traces from files for the e2e tests.

Most critical files:

  • tests/e2e/trace_*.go - the new files in this PR

  • The rest are almost only chore changes to export some fields which otherwise gave me trouble with cmp and Json Marshal/Unmarshaling

  • See an example of the trace format here: tests/e2e/traces/start_provider_chain.json

  • Files in tests/e2e/traces are meant to eventually be input files for e2e tests

  • Files in tests/e2e/tracehandler_testdata are meant to be tests for the parser

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
  • Included the necessary unit and integration tests
  • Included comments for documenting Go code
  • 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 ! in the type prefix if API or client breaking change
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage

@p-offtermatt p-offtermatt self-assigned this Apr 24, 2023
@p-offtermatt p-offtermatt marked this pull request as ready for review April 24, 2023 14:40
@p-offtermatt p-offtermatt linked an issue Apr 24, 2023 that may be closed by this pull request
@p-offtermatt p-offtermatt force-pushed the pofftermatt/875-refactor-existing-e2e-tests-to-take-traces-as-inputs branch from c94894b to ea3cf66 Compare April 25, 2023 12:00
Restructure folder for testdata

Remove ITF writer and start with JSONWriter
Chore: make config fields exported
Marshal Steps with ActionTypes

Debug first version of JSON writer/reader

Remove parser testinputs

Remove outdated package

Tidy go mod

Remove output folder

Resort go.mod

Fix WriteFile call to put low permissions

Inline error check

Improve comments

Add test that writes traces to permanent directory
Add folder with test traces

Add dedicated folder for input traces
@p-offtermatt p-offtermatt force-pushed the pofftermatt/875-refactor-existing-e2e-tests-to-take-traces-as-inputs branch from ea3cf66 to 85f5d15 Compare April 25, 2023 12:10
Copy link
Contributor

@MSalopek MSalopek left a comment

Choose a reason for hiding this comment

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

Thank you for the changes and thank you for cleaning up history and making a concise description of the changes per commit.

Since none of the work outlined here impacts the the actual testing procedure let's proceed with the actual changes. Generally, we should strive for small PRs but in this case most of the complexity comes from having to export all the previously unexported fields to enable JSON unmarshalling.

Approve!

@MSalopek MSalopek merged commit d4417d4 into feat/upgrade-e2e-testing Apr 25, 2023
@MSalopek MSalopek deleted the pofftermatt/875-refactor-existing-e2e-tests-to-take-traces-as-inputs branch April 25, 2023 12:27
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.

Refactor existing e2e tests to take traces as inputs
2 participants