-
-
Notifications
You must be signed in to change notification settings - Fork 44
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
Conversation
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. |
This is a Trojan horse pull request. The PR title clearly states that the purpose is to "test built code". In reality, the PR:
If you're making changes like this, at least be honest about it. There's nothing to be ashamed of, or is there? |
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. |
LGTM |
file: ./reports/lcov.info | ||
tests: | ||
uses: ljharb/actions/.github/workflows/node.yml@a840bfaa7e24d260a9f451baa97ca172fdb327af | ||
with: |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
"engines": { | ||
"node": ">= 0.4" | ||
} |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
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. |
There was a problem hiding this 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", |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
This is effectively the same PR as A11yance/axobject-query#356.