-
Notifications
You must be signed in to change notification settings - Fork 391
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 env for pre-commit tests #1214
base: main
Are you sure you want to change the base?
Conversation
Thank you for making this pull request. Did you know? You can try it on Binder: or . Also, the version of Jupytext developed in this PR can be installed with
(this requires |
cfc6909
to
7f300b5
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1214 +/- ##
==========================================
- Coverage 97.75% 97.66% -0.09%
==========================================
Files 29 29
Lines 4463 4463
==========================================
- Hits 4363 4359 -4
- Misses 100 104 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
30acaa3
to
a759219
Compare
@mwouts I think these changes should make I think CI is failing due to this issue. |
@mwouts So, except for test with pandoc==2.16.2, all of them are passing with new provisioned environment. As this failing test needs a very specific version of I dont really see how we can env var |
a759219
to
4b5cb04
Compare
* Use current test env to run pre-commit tests * This avoids creating a new env and installing jupytext in that env Signed-off-by: Mahendra Paipuri <[email protected]>
* Append hook instead of refering with index Signed-off-by: Mahendra Paipuri <[email protected]>
4b5cb04
to
4c1273b
Compare
Hi @mahendrapaipuri , sorry it took me so long to come back to this PR (BTW I have updated the version of I think my concern is really about allowing the installation of I wondered if we could infer that I also wondered if we could use the |
If we set Rest of techniques involve very huge hacks and that can potentially introduce unwanted side effects on whole The real question we need to ask is if this one test so important that we need to change the behaviour of entire package? If it is, can we pin the version of |
Thanks @mahendrapaipuri ! Yes much better to take the time to think about this. I am not concerned about the tests being slow (although I like this PR that makes them faster!), and I think we should be ready to drop the dependency on The issue that I really want to address is #1210, i.e. Jupytext's pre-commit install fails when node is not available. And when node is available, the pre-commit hook is slow to install (which is why we had this conversation about these tests). I think that the proper fix would be to just no compile the extension when Jupytext is installed as a pre-commit hook, but so far I don't see yet how to achieve that (except by looking at the path for the |
Ahhh, I see what you mean. I thought you were always talking about the pre-commit unit tests. But now I see that you are more worried about using jupytext in the pre-commit hook. Sorry for the miscommunication!! Yes, I agree with you that jupytext should not have to build extension when using in the pre-commit hook. However this is the inconvenience of having a monorepo approach. I don't see any easy way to hack this around just for pre-commit hooks. A straightforward way is to disable extension building by default as I mentioned in my previous comment. Do you see any issues with disabling extension building by default? Another solution is to split the extension into a separate repo! |
jupytext
available in current test environment to execute pre-commit hooks in tests.jupytext
in that environment.Fixes #1210