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 bug in tracking w/ project.refs #1856

Closed
wants to merge 1 commit into from

Conversation

AdrianVovk
Copy link
Contributor

Fixes #1851

@gtristan
Copy link
Contributor

gtristan commented Aug 8, 2023

Hi,

Thanks for this fix !

Can we please put this new test somewhere else instead of a new file named regression.py ? (a large portion of our tests stem from regressions, yet we do not name them as such ;-) ).

In this case, the issue itself is that local sources do not store refs, and as such they leave gaps in the project.refs, causing IndexErrors to occur when tracking.

I think it's safe to say that this test is related to storing refs in project.refs when there is a lacking ref, as such I think it would be good to have this test in tests/project/format.py where we do preliminary testing on project.refs, and perhaps we could name the test test_project_refs_gap() and explain in a comment that this tests a gap in the project.refs file due to the local source not producing a ref.

Noting the issue in the comments around the added test case is also appreciated :)

@gtristan
Copy link
Contributor

@AdrianVovk if you don't want to update this PR, I will grab it and munge your commit (just to rename the test case) and apply this over the weekend.

Hope this is ok with you :)

@AdrianVovk
Copy link
Contributor Author

Sorry bout the delay :) porting to bst2 is taking a bit more work/time than anticipated...

If I don't get to it by then go right ahead!

@gtristan
Copy link
Contributor

If I don't get to it by then go right ahead!

Thanks !

@AdrianVovk
Copy link
Contributor Author

Alright I just moved the test. I couldn't find a tests/project/format.py, but I did find a tests/format/project.py. Let me know if that's not the place you had in mind.

@gtristan
Copy link
Contributor

Alright I just moved the test. I couldn't find a tests/project/format.py, but I did find a tests/format/project.py. Let me know if that's not the place you had in mind.

Right, my bad typo :)

Looks perfect thanks ! I'll get CI to run first but I did run the test locally and was satisfied...

@gtristan
Copy link
Contributor

Closing in favor of #1858

This simply failed the formatting checks, reformatted with tox -e format and passes CI now.

@gtristan gtristan closed this Aug 17, 2023
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.

bst2: Tracking is broken
2 participants