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: add dependency on building @asyncapi/parser for turbo run test #1078

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

aeworxet
Copy link
Contributor

This PR instructs turbo to wait for the build of @asyncapi/parser before executing turbo run test.

Copy link

changeset-bot bot commented Jan 27, 2025

⚠️ No Changeset found

Latest commit: 63db73f

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@aeworxet
Copy link
Contributor Author

This PR is supposed to fix exactly this failing test.

@smoya
Copy link
Member

smoya commented Jan 27, 2025

This PR is supposed to fix exactly this failing test.

I dont understand where the issue was. I see the test of the multiparser passed but not the parser; for some reason it ran for hours.

Would you mind explaining how this change would fix that issue?

@aeworxet
Copy link
Contributor Author

When turbo run test starts executing, the package @asyncapi/parser is not built yet, as a result internal package @asyncapi/parser cannot be found.

image

This change makes task test dependent on successful execution of task @asyncapi/parser#build. Once the internal package @asyncapi/parser is built, test ./packages/multi-parser/test/parse.spec.ts can find it at the specified location and the test doesn't fail.

Copy link
Member

@smoya smoya left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀🌔

@aeworxet
Copy link
Contributor Author

/rtm

@aeworxet
Copy link
Contributor Author

It looks like the merging of this PR is blocked exactly due to the issue it fixes. 😄

2025-01-27T09:24:41.352Z INFO  Merging PR #1078 chore: add dependency on building `@asyncapi/parser` for `turbo run test`
2025-01-27T09:24:41.352Z INFO  Current PR status: mergeable_state: blocked
2025-01-27T09:24:41.353Z INFO  Retrying after 30000 ms ... (1/20)
2025-01-27T09:25:11.785Z INFO  Current PR status: mergeable_state: blocked
2025-01-27T09:25:11.785Z INFO  Retrying after 30000 ms ... (2/20)
2025-01-27T09:25:42.233Z INFO  Current PR status: mergeable_state: blocked
2025-01-27T09:25:42.233Z INFO  Retrying after 30000 ms ... (3/20)

Is there a way to disable this testing for this PR?

@smoya
Copy link
Member

smoya commented Jan 28, 2025

It looks like the merging of this PR is blocked exactly due to the issue it fixes. 😄

2025-01-27T09:24:41.352Z INFO  Merging PR #1078 chore: add dependency on building `@asyncapi/parser` for `turbo run test`
2025-01-27T09:24:41.352Z INFO  Current PR status: mergeable_state: blocked
2025-01-27T09:24:41.353Z INFO  Retrying after 30000 ms ... (1/20)
2025-01-27T09:25:11.785Z INFO  Current PR status: mergeable_state: blocked
2025-01-27T09:25:11.785Z INFO  Retrying after 30000 ms ... (2/20)
2025-01-27T09:25:42.233Z INFO  Current PR status: mergeable_state: blocked
2025-01-27T09:25:42.233Z INFO  Retrying after 30000 ms ... (3/20)

Is there a way to disable this testing for this PR?

Are you sure the turborepo config in use by this PR CI is not the one you modified?

@aeworxet
Copy link
Contributor Author

Are you sure the turborepo config in use by this PR CI is not the one you modified?

Yes, it is exactly the one I modify with this PR:
https://github.com/asyncapi/parser-js/blob/master/.github/workflows/if-nodejs-pr-testing.yml#L73
https://github.com/asyncapi/parser-js/blob/master/package.json#L5

So, currently, the testing of a fix for the testing fails due to a bug in the testing.
Yes, it's a recursion that I'm attempting to fix.

That's why I need to have testing disabled specifically for this PR, so that the testing mechanism is fixed and can pass for all future PRs.

@aeworxet
Copy link
Contributor Author

Current PR testing is done using turbo.json currently present in master:
https://github.com/asyncapi/parser-js/blob/master/turbo.json#L4

My PR modifies it in a way I explained so the testing will pass only for future PRs, not for this one:
https://github.com/asyncapi/parser-js/pull/1078/files#diff-f8de965273949793edc0fbfe249bb458c0becde39b2e141db087bcbf5d4ad5e3R4

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.

3 participants