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

Expose internal events for custom reporters via config #3247

Merged
merged 18 commits into from
Nov 26, 2023

Conversation

codetheweb
Copy link
Contributor

@codetheweb codetheweb commented Sep 11, 2023

We've been using a similar approach to this internally on a patched version of AVA for the last few weeks to pipe test runs to Datadog and it's been working great.

Alternatives:

  • use a Shared Worker: I think the solution here is much simpler than introducing a third type of worker (Shared Workers, Test Workers, and Suite(?) Workers) and covers most use cases I can think of.
  • use Diagnostic Channels: I'm not very familiar with this built-in, but looks like it could help reduce the performance impact of this?

This is very much still a draft, just wanted to get some initial feedback on whether this solution is ok before I continue down the path. Still need to:

  • add TS definitions for internal events (maybe exposed under import type {AVAEvent} from "ava/internal" or something?)
  • add additional tests

@novemberborn
Copy link
Member

I agree that this is the easiest approach that can ship. I don't want to provide any SemVer guarantees to the internal event objects though. This should be an experiment requiring opt-in through the configuration (nonSemVerExperiments: { observeRunsFromConfig: true } } if I recall the syntax).

We've been using a similar approach to this internally on a patched version of AVA for the last few weeks to pipe test runs to Datadog and it's been working great.

Alternatives:

  • use a Shared Worker: I think the solution here is much simpler than introducing a third type of worker (Shared Workers, Test Workers, and Suite(?) Workers) and covers most use cases I can think of.
  • use Diagnostic Channels: I'm not very familiar with this built-in, but looks like it could help reduce the performance impact of this?

This is very much still a draft, just wanted to get some initial feedback on whether this solution is ok before I continue down the path. Still need to:

  • add TS definitions for internal events (maybe exposed under import type {AVAEvent} from "ava/internal" or something?)
  • add additional tests
  • ensure errors in the user's event handler don't affect AVA

Shared workers survive across multiple test runs (when in watch mode) and so would be appropriate. It's just awkward that you have to launch them from a test worker and they could integrate better with the concept of a test run.

lib/cli.js Outdated Show resolved Hide resolved
test/internal-events/fixtures/ava.config.js Outdated Show resolved Hide resolved
@codetheweb codetheweb marked this pull request as draft September 25, 2023 19:25
entrypoints/internal.d.ts Outdated Show resolved Hide resolved
@codetheweb codetheweb force-pushed the feat-expose-internal-events branch from 6930749 to 78f9d37 Compare September 25, 2023 21:57
@codetheweb codetheweb marked this pull request as ready for review September 25, 2023 22:05
import {on} from 'node:events';

export async function * asyncEventIteratorFromApi(api) {
for await (const [plan] of on(api, 'run')) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

is there a scenario where api will emit run multiple times? (other than watch mode maybe?)

Copy link
Member

Choose a reason for hiding this comment

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

No, just in watch mode. May be helpful for the callback function to know that watch mode is active though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 might be good to add as a future enhancement

Copy link
Contributor Author

@codetheweb codetheweb left a comment

Choose a reason for hiding this comment

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

apologies for the late replies!

happy to add more tests, but not sure what to look for

@novemberborn
Copy link
Member

Sorry @codetheweb I've had very little time for open source lately.

Copy link
Member

@novemberborn novemberborn left a comment

Choose a reason for hiding this comment

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

This is looking pretty good, I've added some suggestions. A ton of care went into typing the state change events!

When it comes to additional testing, the good news is that for these "experimental" features the bar is a little lower, so this is pretty close to done IMHO.

entrypoints/internal.cjs Outdated Show resolved Hide resolved
entrypoints/internal.mjs Outdated Show resolved Hide resolved
lib/api-event-iterator.js Outdated Show resolved Hide resolved
entrypoints/internal.d.ts Outdated Show resolved Hide resolved
entrypoints/internal.d.ts Outdated Show resolved Hide resolved
import {on} from 'node:events';

export async function * asyncEventIteratorFromApi(api) {
for await (const [plan] of on(api, 'run')) {
Copy link
Member

Choose a reason for hiding this comment

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

No, just in watch mode. May be helpful for the callback function to know that watch mode is active though.

Copy link
Member

@novemberborn novemberborn left a comment

Choose a reason for hiding this comment

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

Looking good, bar the failing tests :)

lib/api-event-iterator.js Outdated Show resolved Hide resolved
@codetheweb codetheweb force-pushed the feat-expose-internal-events branch from ec70158 to a9eac47 Compare November 9, 2023 20:08
source: ErrorSource | undefined;
};

// eslint-disable-next-line @typescript-eslint/naming-convention
Copy link
Contributor Author

Choose a reason for hiding this comment

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

a new rule started complaining about this, ava is always stylized AVA so I think it's ok to disable here?

Copy link
Member

Choose a reason for hiding this comment

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

I'd like to stick with the rule here. Will push up a commit.

@novemberborn novemberborn merged commit adbfcde into avajs:main Nov 26, 2023
12 of 15 checks passed
@codetheweb codetheweb mentioned this pull request Jan 20, 2024
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.

2 participants