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 refactor-nrepl.find-symbol-test #325

Merged
merged 5 commits into from
Sep 3, 2021
Merged

Add refactor-nrepl.find-symbol-test #325

merged 5 commits into from
Sep 3, 2021

Conversation

vemv
Copy link
Member

@vemv vemv commented Aug 29, 2021

Tries reproducing https://github.com/clojure-emacs/clj-refactor.el/issues/485

find-symbol seemed to lack test coverage anyway.

@expez
Copy link
Member

expez commented Aug 30, 2021

find-symbol seemed to lack test coverage anyway.

We have a lot of various integration tests for this op in the refactor-nrepl.integration-tests ns. That ns runs through the entire middleware, by sending messages like clj-refactor would and complements the more targeted unit tests.

@vemv
Copy link
Member Author

vemv commented Aug 30, 2021

Right, I was grepping for find-symbol instead of find-usages. Thanks!

I'll see if I can repro https://github.com/clojure-emacs/clj-refactor.el/issues/485 by also hacking refactor-nrepl.integration-tests.

@vemv
Copy link
Member Author

vemv commented Aug 30, 2021

The second commit managed to break the build in a pretty wild way, which is both good and bad news 🙃

@vemv vemv force-pushed the find-symbol-test branch from 3b6f4ea to ac4a263 Compare August 31, 2021 16:47
@vemv
Copy link
Member Author

vemv commented Aug 31, 2021

After a few tweaks, the build is green again.

Finally, (set! *warn-on-reflection* true) wasn't causing a test failure whatsoever. It was other stuff failing.

Then how comes https://github.com/clojure-emacs/refactor-nrepl master is green?

I believe it's because of the with_cache CI step. Why are we caching repo again?

Seems really an odd thing to do. It plausibly is caching mranderson's work i.e. the target dir, so test results can get tainted.

@bbatsov
Copy link
Member

bbatsov commented Aug 31, 2021

Yeah, caching the repo seems pretty weird.

vemv added a commit to clojure-emacs/cider-nrepl that referenced this pull request Aug 31, 2021
bbatsov pushed a commit to clojure-emacs/cider-nrepl that referenced this pull request Aug 31, 2021
vemv added 5 commits August 31, 2021 21:00
Tries reproducing https://github.com/clojure-emacs/clj-refactor.el/issues/485

`find-symbol` seemed to lack test coverage anyway.
This appears to cache mranderson's work which is plausibly problematic across CI jobs.
@vemv vemv force-pushed the find-symbol-test branch from ac4a263 to 85649dc Compare August 31, 2021 19:00
@vemv
Copy link
Member Author

vemv commented Aug 31, 2021

PR ready again, and green

The summary is:

  • Add refactor-nrepl.find-symbol-test
  • Add a testing
    • The related deftest was failing too opaquely (when failing)
  • Fix set-refresh-dirs
    • Fixes an issue masked by a prior problem in CI.
  • Make read-ns-form handle invalid files
    • Fixes an issue masked by a prior probem in CI.
  • Remove repo caching
    • This appears to cache mranderson's work, which is plausibly problematic across CI jobs.

Copy link
Member

@bbatsov bbatsov left a comment

Choose a reason for hiding this comment

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

LGTM

@bbatsov bbatsov merged commit b237c20 into master Sep 3, 2021
@bbatsov bbatsov deleted the find-symbol-test branch September 3, 2021 19:22
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