Express ESM versioned tests #1349
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bizob2828
commented
Sep 13, 2022
bizob2828
commented
Sep 13, 2022
| // TODO: remove this when we decide on how to address | ||
| // here: https://issues.newrelic.com/browse/NEWRELIC-3321 | ||
| 'node/no-unsupported-features/es-syntax': 'off', | ||
| 'node/no-unpublished-import': 'off', |
There was a problem hiding this comment.
I will open PR to put this in the shared eslint ruleset. we already do it for the cjs equivalents: https://github.com/newrelic/eslint-config-newrelic/blob/main/index.js#L108
Merged
…lso refactored relying on import + assert type as it is not yet supported in eslint parsing. lastly, renamed test .mjs so eslint can be consistent from root of agent and in test/versioned/express-esm folder
package.json to include the 2 suites we will run. finished porting segments tests, abstracted reusable express helpers
…eatures/es-syntax rule
bizob2828
force-pushed
the
esm-express
branch
from
021cb9f to
3a91273
Compare
jmartin4563
previously approved these changes
Sep 16, 2022
jmartin4563
approved these changes
Sep 16, 2022
Merged
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed Release Notes
Links
Closes https://issues.newrelic.com/browse/NEWRELIC-3119
Details
This will fail until we land the test utils pr and publish a new version. In the meantime you can test this locally by checking out the branch in the test utils PR, running npm link in this branch and then just run
npm run versioned:major. You will see theexpress-esmtests pass. It's also worth noting ESM flavored versioned tests will have some limitations. Since the loader only loads at the start of a test file, we cannot setup and teardown a mocked agent in a before/afterEach hook. Going forward if we add more exclusive ESM versioned tests, the test will need to be made atomic to avoid cross-fire between tests. The most notable changes in this PR are:transactionFinishedhandler and removing after each test. This was the biggest downside of not being able to bootstrap and teardown a new agent between every test.When we originally scoped these port versioned tests to ESM we thought it'd be as easy as swap a few require statements for import and call it a day but it turned out to be a large effort. That's why I did not port every single test from the cjs express tests. Verifying both transaction and segment naming tests work is a good compromise and gives me enough confidence that the agent ESM loader is properly tested via integration tests.