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

test-var-query does not run tests with + in the name #730

Closed
bpringe opened this issue Nov 24, 2021 · 3 comments
Closed

test-var-query does not run tests with + in the name #730

bpringe opened this issue Nov 24, 2021 · 3 comments

Comments

@bpringe
Copy link

bpringe commented Nov 24, 2021

Expected behavior

Tests with + in the name are run successfully.

Actual behavior

Tests with + in the name are not run successfully.

Steps to reproduce the problem

Please see this Calva issue here for details: BetterThanTomorrow/calva#1383.

It seems that if a + is in the middle of the test name, the test isn't found. If the + is at the beginning of the test name, an exception is thrown in the repl.

In Calva, we send the test as below, and I verified that test has the appropriate test name - that it was not altered by Calva before being send to cider-nrepl.

            this.client.write({
                op: "test-var-query",
                ns,
                id,
                session: this.sessionId,
                "var-query": {
                    "ns-query": {
                        exactly: [ns]
                    },
                    search: test,
                    "search-property": "name"
                }
            });

Environment & Version information

cider-nrepl version

0.26.0

Java version

openjdk 16.0.2 2021-07-20
OpenJDK Runtime Environment Temurin-16.0.2+7 (build 16.0.2+7)
OpenJDK 64-Bit Server VM Temurin-16.0.2+7 (build 16.0.2+7, mixed mode, sharing)

Operating system

MacOS Monterey

@bbatsov
Copy link
Member

bbatsov commented Nov 24, 2021

My guess is that + is messing up the var-query, but I'll have to test it explicitly. The old more specific test ops were deprecated in favor of test-var-query, as it provides all of their functionality and more.

@vemv
Copy link
Member

vemv commented Nov 25, 2021

I've tested orchard.query/vars which is what test-var-query uses and it appears to work just fine:

image

If you pass a + it should be escaped (presumably client-side: we shouldn't second-guess what a client meant). Pattern/quote can be useful, for example.

Does that make sense, @bpringe ?

@bpringe
Copy link
Author

bpringe commented Nov 27, 2021

Ah, yes that does make sense. Thanks! I'll close this, then.

@bpringe bpringe closed this as completed Nov 27, 2021
marcomorain added a commit to marcomorain/calva that referenced this issue Nov 30, 2021
Per clojure-emacs/cider-nrepl#730, clients
should make sure that queries are escaped. We can use the
`escape-string-regexp` package, which is already in the source-tree for
this.

Also, change the call to `test-var-query` to narrow the search to just
`test?` true, and remove `search-property`, since `name` is the default
per the Cider docs.

Fixes BetterThanTomorrow#1383
marcomorain added a commit to marcomorain/calva that referenced this issue Dec 1, 2021
Per clojure-emacs/cider-nrepl#730, clients
should make sure that queries are escaped. Added a function to
utilities.ts to do this.

Also, change the call to `test-var-query` to narrow the search to just
`test?` true, and remove `search-property`, since `name` is the default
per the Cider docs.

Fixes BetterThanTomorrow#1383
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

No branches or pull requests

3 participants