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

Fix running Leiningen eftest task with explicitly specified Leiningen profile #40

Conversation

gsnewmark
Copy link
Contributor

Currently to run eftest Leiningen task with explicit Leiningen profile (using with-profile) it's required to also explicitly include leiningen/test profile (which includes default test selector selecting all available tests) or explicitly specifying custom test selector.

For example this won't find any tests:

gsnewmark@asgard:/tmp/eftest-profiles|
⇒  lein with-profiles dev eftest
No tests found.

But adding leiningen/test will find all tests:

gsnewmark@asgard:/tmp/eftest-profiles|
⇒  lein with-profiles dev,leiningen/test eftest

FAIL in eftest-profiles.core-test/a-test (core_test.clj:7)                      
FIXME, I fail.
expected: 0
  actual: 1


1/1   100% [==================================================]  ETA: 00:00

Ran 1 tests in 0,028 seconds
1 assertion, 1 failure, 0 errors.
Tests failed.
Error encountered performing task 'eftest' with profile(s): 'dev,test'
Tests failed.

Reproducible example (just the default Leiningen-generated project with added eftest plugin) https://github.com/gsnewmark/eftest-profiles

To be consistent with default Leiningen test runner, which doesn't require specifying leiningen/test every time, we need to implicitly merge test-specific profiles into project before calculating test selectors, not just before test form generation.

@weavejester
Copy link
Owner

Thanks for the fix. Can you change the commit message to adhere to the seven rules. So perhaps something like:

Add implicit :leiningen/test profile to plugin

Allows Leiningen plugin to be run on profiles without needing to
explicitly include the :leiningen/test profile.

@gsnewmark gsnewmark force-pushed the fix-proper-support-for-lein-profiles branch from 0de216c to fa4d349 Compare March 6, 2018 15:51
@gsnewmark
Copy link
Contributor Author

gsnewmark commented Mar 6, 2018

I've updated the message

@weavejester
Copy link
Owner

Can you explain this part of the commit message:

without needing to explicitly include the eftest dependency

@gsnewmark
Copy link
Contributor Author

'Profile' for eftest is also included implicitly right away, and it includes the eftest dependency https://github.com/gsnewmark/eftest/blob/fa4d3496e1f1c359ab31ca86a6e31b274d113358/lein-eftest/src/leiningen/eftest.clj#L7-L8.

I can remove mention of it if you'd like 🙂

Allows Leiningen plugin to be run on profiles without needing to
explicitly include the :leiningen/test profile.
@gsnewmark gsnewmark force-pushed the fix-proper-support-for-lein-profiles branch from fa4d349 to 7384fb4 Compare March 6, 2018 19:57
@gsnewmark
Copy link
Contributor Author

Updated.

@weavejester
Copy link
Owner

weavejester commented Mar 7, 2018

You didn't need to change it, I just wanted to understand what it meant. :)

@weavejester weavejester merged commit e3878e8 into weavejester:master Mar 7, 2018
@weavejester
Copy link
Owner

I'll cut a new release in a little while.

@gsnewmark
Copy link
Contributor Author

Thanks! It was actually a bit confusing and didn't add much to understanding of the fix, so I've switched to your variant of the commit messabe 🙂

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.

2 participants