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

Add test ns query under var-query field #1983

Merged
merged 3 commits into from Dec 3, 2022

Conversation

deadeyejoe
Copy link
Contributor

Closes #1981.

What has changed?

Running a single test would result in every test in every namespace being run. This was because the test var query was being spread into the nrepl payload, rather than being passed as 'var-query'.

Looks to have been introduced by commit f7844f3.

My Calva PR Checklist

If this PR involves only documentation changes, I have:

  • [ ~] Read Editing Documentation
  • [ ~] Directed this pull request at the published branch.
  • [ ~] Built the site locally (if the changes were more involved than simple typo fixes), and verified that the site is presented as expected.
  • [ ~] Referenced the issue I am fixing/addressing in a commit message for the pull request (if there was is an issue for the documentation change)
    • [~] If I am fixing the issue, I have used GitHub's fixes/closes syntax
    • [~] If I am fixing just part of the issue, I have just referenced it w/o any of the "fixes” keywords.

If this PR involves code changes, I have:

  • Read How to Contribute.
  • Directed this pull request at the dev branch. (Or have specific reasons to target some other branch.)
  • Made sure I have changed the PR base branch, so that it is not published. (Sorry for the nagging.)
  • Updated the [Unreleased] entry in CHANGELOG.md, linking the issue(s) that the PR is addressing.
  • Figured if anything about the fix warrants tests on Mac/Linux/Windows/Remote/Whatever, and either tested it there if so, or mentioned it in the PR.
  • [~] Added to or updated docs in this branch, if appropriate
  • [~] Tests
    • Tested the particular change
    • Figured if the change might have some side effects and tested those as well.
    • Smoke tested the extension as such.
    • Tested the VSIX built from the PR (so, after you've submitted the PR). You'll find the artifacts by clicking Show all checks in the CI section of the PR page, and then Details on the ci/circleci: build test.
  • Referenced the issue I am fixing/addressing in a commit message for the pull request.
    • If I am fixing the issue, I have used GitHub's fixes/closes syntax
    • [~] If I am fixing just part of the issue, I have just referenced it w/o any of the "fixes” keywords.
  • [~] Created the issue I am fixing/addressing, if it was not present.
  • Formatted all JavaScript and TypeScript code that was changed. (use the prettier extension or run npm run prettier-format)
  • Confirmed that there are no linter warnings or errors (use the eslint extension, run npm run eslint before creating your PR, or run npm run eslint-watch to eslint as you go).

Ping @PEZ, @bpringe, @corasaurus-hex, @Cyrik

@PEZ
Copy link
Collaborator

PEZ commented Nov 29, 2022

Thanks for contributing! 🙏 ❤️

By the wording in the PR, it seems like you have found the right place to fix it. Haven't had a look yet. Will tomorrow! Prepping for this event right now: https://www.meetup.com/london-clojurians/events/286030325/ 😄

CHANGELOG.md Outdated Show resolved Hide resolved
@bpringe
Copy link
Member

bpringe commented Nov 29, 2022

@PEZ I'm not sure why that build-test workflow fails while packaging the vsix.

[webpack-cli] Error: error:0308010C:digital envelope routines::unsupported

@PEZ
Copy link
Collaborator

PEZ commented Nov 30, 2022

@PEZ I'm not sure why that build-test workflow fails while packaging the vsix.

[webpack-cli] Error: error:0308010C:digital envelope routines::unsupported

That is a very, very strange, and worrying, error. I'll have a look later today.

@PEZ
Copy link
Collaborator

PEZ commented Nov 30, 2022

I find this issue, which seems to have cause a ruckus in the Webpack community:

It is many, many pages of comments. Seems like the conclusion is to upgrade to Webpack 5. Which sucks a bit for us, because that is what we are using! But anyway, now I am not as scared by this error. I'm sure we'll at least find a workaround with some sensible amount of work.

@PEZ
Copy link
Collaborator

PEZ commented Nov 30, 2022

I've reproduced the error in a separate branch. Let's see if I can figure out a way to fix it.

PEZ added a commit that referenced this pull request Nov 30, 2022
PEZ added a commit that referenced this pull request Nov 30, 2022
Fixes #1985

See also: #1983
@PEZ
Copy link
Collaborator

PEZ commented Nov 30, 2022

OK, sorry that your PR got a bit messy with the Webpack business, @deadeyejoe. But now we have a workaround/maybe-fix on dev, so please merge that and I think (hope) this build will turn green.

deadeyejoe and others added 3 commits November 30, 2022 22:45
To fix issue 1981. Runnning a single test would result in every
test in every namespace being run. This was because the test var query
was being spread into the nrepl payload, rather than being passed as
'var-query'.

Looks to have been introduced by commit f7844f3.
Removed reference to polylith as this affects all project types
@deadeyejoe
Copy link
Contributor Author

Fingers crossed!

@bpringe bpringe merged commit b83bcd0 into BetterThanTomorrow:dev Dec 3, 2022
@bpringe
Copy link
Member

bpringe commented Dec 3, 2022

Thanks again, @deadeyejoe!

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.

None yet

3 participants