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

run tests on built test files, test on all supported nodes #567

Merged
merged 4 commits into from
Sep 12, 2024
Merged

Conversation

ljharb
Copy link
Collaborator

@ljharb ljharb commented Jul 15, 2024

This is effectively the same PR as A11yance/axobject-query#356.

@ljharb ljharb requested a review from marcysutton July 15, 2024 19:14
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@wojtekmaj
Copy link

wojtekmaj commented Jul 16, 2024

This is a Trojan horse pull request. The PR title clearly states that the purpose is to "test built code". In reality, the PR:

  • Sneaks in support for Node 0.4 (I was only able to track down an intention to support Node 6: Convert several maps to use objects #162 (comment))
  • Replaces fundamental GitHub Actions with ones you control
  • Changes the testing framework to one you control
  • Adds a bunch of new dev dependencies that you control

If you're making changes like this, at least be honest about it. There's nothing to be ashamed of, or is there?

@AaronDewes
Copy link

To be fair, the actions are pinned to a commit. There isn't a risk of malicious code being added there. The deps are more dsngerous though, but there is always some trust in maintainers involved.

@yamiteru
Copy link

LGTM

file: ./reports/lcov.info
tests:
uses: ljharb/actions/.github/workflows/node.yml@a840bfaa7e24d260a9f451baa97ca172fdb327af
with:

This comment was marked as off-topic.

@HawkeyePierce89

This comment was marked as off-topic.

Comment on lines +67 to +69
"engines": {
"node": ">= 0.4"
}

This comment was marked as abuse.

@nitelite

This comment was marked as off-topic.

@ljharb
Copy link
Collaborator Author

ljharb commented Jul 16, 2024

These are all dev deps, and as such, it doesn’t make sense for anyone but maintainers to have an opinion on them.

@wojtekmaj there’s nothing dishonest here, and accusing me of doing something “sneaky” is an absurd accusation.

@A11yance A11yance locked as too heated and limited conversation to collaborators Jul 16, 2024
Copy link
Member

@marcysutton marcysutton left a comment

Choose a reason for hiding this comment

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

Hey @ljharb, thanks for the PR. This looks good! There are still no dependencies and the changes are really for testing against a variety of Node engines. It follows the approach of axobject-query which seems fine to me.

"commander": "^9.4.1",
"eslint": "^8.26.0",
"eslint-config-airbnb-base": "^15.0.0",
"deep-equal-json": "^1.0.0",
Copy link
Member

Choose a reason for hiding this comment

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

@ljharb do you need deep-equal-json here or can you make a similar change to A11yance/axobject-query#357? (I see that it's only a devDependency)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

aria-query is definitely going to end up with zero deps, but since the replacement function isn't exposed (intentionally), i can either use the dev dep, or i can duplicate the code, and the dev dep felt simpler.

@ljharb ljharb merged commit 0c0d722 into main Sep 12, 2024
69 checks passed
@ljharb ljharb deleted the updates branch September 12, 2024 21:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants