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

[chore] update e2e tests to use BATS assertions #121

Merged
merged 13 commits into from
May 16, 2023

Conversation

robbkidd
Copy link
Member

@robbkidd robbkidd commented Apr 28, 2023

Short description of the changes

  • Use bats for e2e testing
  • Modify step in workflow to use BATS to make more precise assertions about the unredacted trace data
  • Moves the redaction logic for the traces.json file to the BATS test helpers and asserts no-git-difference in the test suites
  • Add utilities test helper for more clarity (specifically includes named functions that are easier to reason about)

This can be run locally as well. You will get a traces-orig.json file for each test run, and from there you can further work with the tests by running bats verify.bats in the directory if you install bats locally.

Note I added a TODO for gorilla/mux spans coming out with a scope name of net/http, probably as a result of being produced by the net/http probe. This is a PR for updating tests, so I propose fixing the probe source is out of scope and should be addressed in a separate PR.

How to verify that this has the expected result

e2e tests pass with expected results. Running locally with make fixture-nethttp and the others has expected results, and also creates a clean traces.json with expected output.

To see an example of a fail locally, run one of the e2e tests (e.g. make fixture-gin), change directory to test/e2e/gin, change the local traces-org.json (maybe change the real span ID to "nope"), and run bats verify.bats.

e2e-fail-gin

@robbkidd
Copy link
Member Author

First learning from the experiment: bats is not already on the runner.

@robbkidd
Copy link
Member Author

Example output from BATS (not all real tests):

Screenshot 2023-04-28 at 4 16 27 PM

@MikeGoldsmith MikeGoldsmith changed the title wip: e2e experiment with BATS [chore] update e2e tests to use BATS assertions May 4, 2023
@MikeGoldsmith MikeGoldsmith marked this pull request as ready for review May 4, 2023 17:07
@MikeGoldsmith MikeGoldsmith requested a review from a team as a code owner May 4, 2023 17:07
@JamieDanielson
Copy link
Member

The second note I added above "2) http span names do not match latest conventions" is fixed with #143 . I can update that after this gets merged or vice versa.

@JamieDanielson
Copy link
Member

The first note I added above "1) gorilla/mux spans are actually using the net/http probe" is partially fixed with #86 . There is still an extra net/http span there, but at least gorilla/mux scope will show up as expected.

Copy link
Member

@MikeGoldsmith MikeGoldsmith left a comment

Choose a reason for hiding this comment

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

I think this is great and we should take this PR as is, then update the tests in the other two PRs to use BATS.

@robbkidd
Copy link
Member Author

robbkidd commented May 9, 2023

What do y'all think about removing the tests/e2e/*/traces.json files and adding them to .gitignore with this PR? With specific test assertions about expected state of distinct portions of the JSON, the files don't need to be tracked by version control to support e2e test runs.

Update: upgrading this from a suggestion to a recommendation as I see my attempts to work on #106 do not produce a traces.json by the collector (I'm probably not emitting the spans I think I am) and the kubectl cp command does not exit non-zero when the copy fails.

❯ kubectl cp -c filecp default/test-opentelemetry-collector-0:tmp/trace.json ./test/e2e/grpc/traces.json
tar: tmp/trace.json: No such file or directory
tar: error exit delayed from previous errors
❯ echo $?
0

The tests continue past this failed copy and, if the Known Good JSON remains present, the assertions about the trace data produced are checking the Known Good state.

@MikeGoldsmith
Copy link
Member

Does removing the traces.json still allow a jq comparision check? The BATS assertions are good for determining whether something expect is present but we can't check for unexpected data, eg additional span attributes. The jq comparison allows us to check the complete shape of the generated telemetry, with a few redacted values.

Should we care about extra attributes if the ones we expect to be present are?

@robbkidd
Copy link
Member Author

Alrightie!

Latest change:

  • brings back the redaction jq command as a function within the BATS utilities
  • one of the tests in each library's e2e suite is to perform the JSON redaction and assert no git diff
  • removes the traces.json file before running the tests (and before the redaction-jq generates it again) to protect the e2e checks from verifying the already-known-good state that is checked in to version control

I've held off adding an e2e suite for grpc because that instrumentation doesn't pass the assertions that the others do, yet. I reckon that work can be in a follow-up PR.

@MasonLegere
Copy link

Unrelated to the scope of the tests but might be nice given that the action is taking ~5 minutes :)

concurrency:
  group: ${{ github.ref }}
  cancel-in-progress: true

@robbkidd
Copy link
Member Author

Would that addition be to .github/workflows/kind.yaml like so?

#
  jobs:
    kubernetes-test:
+     concurrency:
+       group: ${{ github.ref }}
+       cancel-in-progress: true
      strategy:
        matrix:
          k8s-version: ["v1.26.0"]
          library: ["gorillamux", "nethttp", "gin"]
#

And if I understand that correctly, it would cancel all of these e2e tests if any faster check against the same commit fails, right?

Can the CI config change happen on a separate PR? Or does it block acceptance of this one?

@MasonLegere
Copy link

MasonLegere commented May 12, 2023

The addition would be at the workflow level as opposed to the job and added on line 10. The impact is pretty small and only applies in the case where a commit is pushed quickly after another. The new commit would trigger all the new e2e tests to re-run where most likely the runners from the previous commit should be cancelled to save runner minutes if they're still active.

It's pretty small and might not come up frequently but I can add it as a separate PR if there is some interest

For the behavior you were suggesting the fail-fast: true flag can be added to the job as well

@robbkidd
Copy link
Member Author

It sounds like a good idea. I would support it† in a separate PR so that the conversation around adding short-circuits to CI can be distinct from the conversation here about an overhaul of end-to-end test assertions.

† Disclosure: I am a new contributor to this project, so my support is of questionable value.

@robbkidd
Copy link
Member Author

robbkidd commented May 12, 2023

@open-telemetry/go-instrumentation-approvers I believe this is ready and worthy of review. 🙇🏻 💌 💞

Update: 🤦🏻 Except it needs to be brought up-to-date with main. On it!

Update to the update: it's up-to-date again.

@robbkidd robbkidd force-pushed the robb.smoke-em-if-you-got-em branch from 3263607 to ab8f6ef Compare May 12, 2023 16:49
@robbkidd
Copy link
Member Author

Updated the PR description text as well, so that it might work as the body of the message for a merge commit for this PR.

Copy link
Contributor

@edeNFed edeNFed left a comment

Choose a reason for hiding this comment

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

Looks great!
I think we have enough approvers, can we proceed to merge this?

@robbkidd robbkidd force-pushed the robb.smoke-em-if-you-got-em branch from 9f38d73 to a664dec Compare May 15, 2023 15:40
JamieDanielson and others added 7 commits May 15, 2023 13:17
spans currently show net/http, will need review for lib name
PR open-telemetry#143 updated these span names to include HTTP method per the spec.
+ remove before verifying with e2e assertions
+ bring back redaction-jq as a function within the BATS utilities
+ assert no git difference with a BATS test
@JamieDanielson
Copy link
Member

Updated this PR to include the proper tests for gorillamux now that #86 has been merged in.

@robbkidd robbkidd requested a review from damemi May 15, 2023 19:13
@MikeGoldsmith
Copy link
Member

@open-telemetry/go-instrumentation-approvers This is ready for more eyes before we can merge.

Makefile Show resolved Hide resolved
@MrAlias MrAlias merged commit 3744384 into open-telemetry:main May 16, 2023
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants