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(commit): correct the stage checker before commiting #420

Merged
merged 2 commits into from
Sep 12, 2021

Conversation

xaviml
Copy link
Contributor

@xaviml xaviml commented Sep 11, 2021

fixes #418

Checklist

  • Add test cases to all the changes you introduce
  • Run ./script/format and ./script/test locally to ensure this change passes linter check and test
  • Test the changes on the local machine manually
  • Update the documentation for the changes

This part of the code was not originally tested, since changing the code did not break the existing tests. If interested, I could add tests for this with real git calls.

I did not add this to the documentation or README since it fixes a behaviour that was supposed to happen already.

@codecov
Copy link

codecov bot commented Sep 11, 2021

Codecov Report

Merging #420 (a2d6312) into master (dc6e4b0) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #420      +/-   ##
==========================================
- Coverage   97.90%   97.90%   -0.01%     
==========================================
  Files          39       39              
  Lines        1383     1382       -1     
==========================================
- Hits         1354     1353       -1     
  Misses         29       29              
Flag Coverage Δ
unittests 97.90% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
commitizen/git.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 79b3156...a2d6312. Read the comment docs.

@woile
Copy link
Member

woile commented Sep 12, 2021

Hi @xaviml thanks for the contribution!
I do not understand the issue, so I'd like to ask you a test where it would fail before this PR and fixed after. It would also work as a way to document the behavior, and by seen it, I think I would understand better 👍🏻

Sorry for the inconvinience 😅

@xaviml
Copy link
Contributor Author

xaviml commented Sep 12, 2021

Hi @woile ,

Of course, I totally understand. Is there any test example in the test suite that actually test with the real git command? Is it okay if I do so? Otherwise, I don't see a way to test this properly since it only calls the git command.

Regards,
Xavi M.

@woile
Copy link
Member

woile commented Sep 12, 2021

You can write it in https://github.com/commitizen-tools/commitizen/blob/master/tests/test_git.py

You can call git with this:

cmd.run("git config log.showsignature true")

By using this fixture you'll have a git folder ready:

@pytest.mark.usefixtures("tmp_commitizen_project")

Example:

@pytest.mark.usefixtures("tmp_commitizen_project")
def test_git_something():
   c = cmd.run("git something")

@xaviml
Copy link
Contributor Author

xaviml commented Sep 12, 2021

Hi @woile,

Thank you for pointing me to the right test file. I did the following:

  • Rename existing is_staging_clean test to test_is_staging_clean_when_adding_file
  • Add test_is_staging_clean_when_updating_file

The new test fails in the second assert with the old code. The new code now checks correctly if stage is clean or not.

The bug was (as described in #418) that the is_stage_clean worked when adding a file (as it was tested), but not when modifying an existing file (it was not tested).

Let me know if anything needs changing.

Regards,
Xavi M.

@woile
Copy link
Member

woile commented Sep 12, 2021

I see it now, wow I thought this was working.
Thanks!

@woile woile merged commit a288fd9 into commitizen-tools:master Sep 12, 2021
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.

File stage checker not working as expected
2 participants