Skip to content

Conversation

nprizal
Copy link
Contributor

@nprizal nprizal commented Jan 23, 2023

Description

Currently, test-collector-javascript for Jest sends a full failure message as failure_reason and skips failure_expanded. It should send a full failure message as failure_expanded and send a summary as failure_reason.

Context

We want to limit the length of failure_reason that can be stored by Test Analytics in the upcoming changes. Therefore, we need to split the failure messages into a short summary that goes into failure_reason and the detail that goes into failure_expanded

Changes

  1. jest reporter will now send the first line of failureMessages as failureReason and the rest of the lines as failureExpanded (resolves Send failure_expanded on Jest failures #17)
  2. Test analytics API endpoint is now configurable with BUILDKITE_ANALYTICS_URL env (resolves Support configurable API endpoint URL for debugging purposes #46)

Verification

Test the integration with Test Analytics API by following these steps:

  1. install dependencies npm install
  2. go to the example directory cd examples/jest
  3. run the example test BUILDKITE_ANALYTICS_TOKEN=<YOUR_SUITE_TOKEN> npm test
  4. you should be able to see the results in Test Analytics

note:
If you want to verify against local Test Analytics, add BUILDKITE_ANALYTICS_URL=http://analytics-api.buildkite.localhost/v1/uploads env when running the command:

BUILDKITE_ANALYTICS_TOKEN=<YOUR_SUITE_TOKEN> BUILDKITE_ANALYTICS_URL=http://analytics-api.buildkite.localhost/v1/uploads npm test

Deployment

Publish to NPM registry

@nprizal nprizal self-assigned this Jan 24, 2023
@nprizal nprizal requested a review from a team January 24, 2023 03:38
@nprizal nprizal marked this pull request as ready for review January 24, 2023 03:38
Copy link
Contributor

@niceking niceking left a comment

Choose a reason for hiding this comment

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

Hmm, I don't think I know enough about Jest to approve this review, but the approach looks sensible, and is similar to what the ruby test collector does.

One thing I was wondering was if it was possible to integration test this before the package was released, but I have no idea how you'd do this in node. Like doing the node equivalent of bundling to a local version of test collector ruby and running the test and seeing if the right output is generated. Might be sending you on a wild goose chase tho!

plugins:
- docker#v3.7.0:
image: node:latest
image: node:lts
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, is this image change and the node version bump essential for this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I see that you merged a branch in with these changes to get the tests to pass. Perhaps it would be good to call that out and merge the other branch before this one? Or perhaps rebase this branch onto the branch with the node update?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@niceking Good point! I tested the integration with Test Analytics locally (i.e. running example tests and sending the results to local analytics API), and I can write documentation for that. But I think it also worth making the API URL configurable to easily debug/test the integration locally; similar to what we did for Ruby collector)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@niceking as for the node update, I'll make sure to merge the other one first and rebase this branch. I'll put this PR back to draft.

@nprizal nprizal marked this pull request as draft January 24, 2023 04:18
@nprizal nprizal force-pushed the pie-1380-send-failure_expanded-from-jest branch from 90076e9 to c67ce1f Compare January 24, 2023 20:54
@nprizal nprizal requested a review from niceking January 24, 2023 21:23
@nprizal nprizal marked this pull request as ready for review January 24, 2023 21:23
@nprizal nprizal requested a review from a team January 24, 2023 21:23
@niceking
Copy link
Contributor

Perhaps its my unfamiliarity with the npm development pipeline, but I don't believe these instructions are sufficient for locally testing a package under development?

Test the integration with Test Analytics API by following these steps:

  1. install dependencies npm install
  2. go to the example directory cd examples/jest
  3. run the example test BUILDKITE_ANALYTICS_TOKEN=<YOUR_SUITE_TOKEN> npm test
  4. you should be able to see the results in Test Analytics

note:
If you want to verify against local Test Analytics, add BUILDKITE_ANALYTICS_URL=http://analytics-api.buildkite.localhost/v1/uploads env when running the command:

I believe that doing npm install will be pulling the released version of the jest test collector from the npm package registry. I think we'd have to do something like npm install path/to/your/package to run a local version of the package. I pulled this from this post which has a number of different strategies for testing packages.

Did you confirm that running the jest collector this way locally generates the appropriate S3 file in minio locally, and the data for failure_expanded is now populated?

}

axios.post('https://analytics-api.buildkite.com/v1/uploads', data, config)
const buildkiteAnalyticsUrl = process.env.BUILDKITE_ANALYTICS_URL || DEFAULT_BUILDKITE_ANALYTICS_URL
Copy link
Contributor

Choose a reason for hiding this comment

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

This definitely makes testing easier, nice!

@nprizal
Copy link
Contributor Author

nprizal commented Jan 25, 2023

I believe that doing npm install will be pulling the released version of the jest test collector from the npm package registry. I think we'd have to do something like npm install path/to/your/package to run a local version of the package. I pulled this from this post which has a number of different strategies for testing packages.

@niceking Thanks for the feedback 💚. The examples are configured as npm workspace (see the doc). Running npm install from the root package will install dependencies for all workspaces. In each workspace, we add buildkite-test-collector) as a dependency by referring it directly to the local directory instead of the npm registry (see package.json for examples/jest). When you run npm test from examples/jest, it will use buildlite-test-collector from your local directory.

Did you confirm that running the jest collector this way locally generates the appropriate S3 file in minio locally, and the data for failure_expanded is now populated?

I checked what the collector is sending to the analytics API and can see the failure_expanded in the payload. However, I didn't check the S3 file in minio. (can you guide me how to do this?)

@nprizal nprizal requested a review from niceking January 25, 2023 02:42
@niceking
Copy link
Contributor

I checked what the collector is sending to the analytics API and can see the failure_expanded in the payload. However, I didn't check the S3 file in minio. (can you guide me how to do this?)

Cool! Actually, I don't even know if the jest collector is set up to operate with our local minio set up, so this could just be a misunderstanding on my part :) Would still love a zoom to sync on this though just so I understand how the local package development works!

@nprizal nprizal merged commit 20890f0 into main Jan 25, 2023
@nprizal nprizal deleted the pie-1380-send-failure_expanded-from-jest branch January 25, 2023 21:16
@iampatbrown
Copy link

@nprizal Is it fine to use BUILDKITE_ANALYTICS_URL for overriding the URL? I was planning to add BUILDKITE_ANALYTICS_BASE_URL to the Swift collector and realised this might lead to an inconsistency between the collectors.

From the docs:

BUILDKITE_ANALYTICS_URL = A URL that links to the build in your CI
example: https://ci.example.com/project1/runs/a086005a/attempts/4

@blaknite
Copy link
Member

@iampatbrown 💯 that's been overloaded and will break the collector if used. Thanks 🧡

@nprizal is on it!

@nprizal
Copy link
Contributor Author

nprizal commented Jan 26, 2023

@iampatbrown thanks for the feedback 💚. It has been fixed in v1.4.1.

@iampatbrown
Copy link

Awesome :) thanks @blaknite and @nprizal

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.

Support configurable API endpoint URL for debugging purposes Send failure_expanded on Jest failures

4 participants