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

Use test-var-query for all test commands #1424

Merged
merged 3 commits into from
Dec 11, 2021

Conversation

marcomorain
Copy link
Contributor

@marcomorain marcomorain commented Dec 7, 2021

What has Changed?

Remove use of deprecated Cider commands, in favour of the supported
test-var-query. This fixes an issue with cljc conditionals impacting the
ability to run tests.

Fixes: #1328

My Calva PR Checklist

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
  • 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. NB: You need to sign in/up at Circle CI to find the Artifacts tab.
    • Tested the particular change
    • Figured if the change might have some side effects and tested those as well.
    • Smoke tested the extension as such.
  • 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.

Ping @PEZ, @bpringe

@bpringe
Copy link
Member

bpringe commented Dec 8, 2021

Seems to work in my simple test 😃 . I'm not sure if you're done with the changes yet, but if so, once the changelog is updated and vsix is tested I think we can merge.

Remove use of deprecated Cider commands, in favour of the supported
test-var-query. This fixes an issue with cljc conditionals impacting the
ability to run tests.

Fixes: BetterThanTomorrow#1328
@marcomorain
Copy link
Contributor Author

This should be good merge @bpringe 👍

@marcomorain
Copy link
Contributor Author

I'm not sure about how to move forward with this – this is a behaviour change - the testAll command currently loads all namespace, but this PR changes that behaviour.

@PEZ
Copy link
Collaborator

PEZ commented Dec 10, 2021

Is it that the new op does not accept the load? option?

@marcomorain
Copy link
Contributor Author

Yup, that’s exactly it.

@PEZ
Copy link
Collaborator

PEZ commented Dec 10, 2021

I say, go for this. Your activities around this whole test story is improving things majorly and some behavior changes are inevitable. We can't afford to hang on to deprecated functionality. Maybe consider adding something to calva.io about that loading is a user responsibility?

@bpringe
Copy link
Member

bpringe commented Dec 11, 2021

I'm not sure this actually changes behavior. For example, in version 2.0.228 of Calva,I have the following test code:

(ns test-lein.core-test
  (:require [clojure.test :as test :refer [deftest is]]))

(deftest test-1
  (is true))

(deftest test-2
  (is true))

and I do the following:

  1. Run all tests, see they all pass
  2. Change true to false in test-2
  3. Run all tests again

Whether I save the file or not at step 2, all tests continue to pass, even though I changed test-2 to make it fail. Furthermore, even if I run Refresh All Namespaces or Refresh Changed Namespaces then run all tests again, the change still isn't picked up.

That last issue may be a problem with the refresh commands. I don't normally use them, but they don't seem to work as I expect in this case.

My overall point is that currently it doesn't seem that changes are loaded before tests are run anyway, at least not in this lein project I'm testing with. I have to run the command to load the test file in order for changes in tests to be picked up. I thought that I've observed otherwise in the past though 🤔 . I tested also with a deps.edn project and observed the same behavior.

@marcomorain
Copy link
Contributor Author

Sounds like we should merge away then?

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.

3 participants