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

[MRG] Ensure git submodules are updated and initialized #639

Conversation

@djhoese
Copy link
Contributor

djhoese commented Apr 7, 2019

This is a continuation of #540. See that PR for details. That PR by @alimanfoo was only missing a test so this is my attempt at adding one.

Some things to note:

  1. Should I merge or rebase upstream or leave this branch where it is?
  2. I'm not a pytest fixture expert, but I don't think there was a cleaner way of doing what I needed to do to create the git repositories. I separated out the functionality of repo_with_content so I could reuse them in my new fixture.
  3. After creating the fixture, I then realized there was no builtin way to get the submodule's SHA without running some commands with subprocess. It made the git unit tests uglier, but I wasn't sure what else to do.
  4. As part of point 2 above, I changed the functionality of adding content to a test repository to use append so theoretically someone could call the same method multiple times to create multiple commits.

I think this tests what it needs to test, but may not be how the maintainers want this structured. Let me know what you want changed. Git tests pass locally.

@djhoese djhoese changed the title [MRG] Git update submodules alimanfoo 20181224 merry xmas [MRG] Ensure git submodules are updated and initialised Apr 7, 2019
@djhoese djhoese changed the title [MRG] Ensure git submodules are updated and initialised [MRG] Ensure git submodules are updated and initialized Apr 7, 2019
@@ -18,6 +19,28 @@ def test_clone(repo_with_content):
assert git_content.content_id == sha1[:7]


def test_submodule_clone(repo_with_submodule):
"""Test git clone containing a git submodule."""
upstream, sha1_upstream, sha1_submod = repo_with_submodule

This comment has been minimized.

Copy link
@betatim

betatim Apr 8, 2019

Member

Maybe we can prefix these with expected_? I think the similar variable names to the ones we get from the actually checked out repo are the source of the assert statement mixup below. Definitely took me a while to spot that sha1_submod and submod_sha1 where not the same name :)

@betatim

This comment has been minimized.

Copy link
Member

betatim commented Apr 8, 2019

This looks good to me. Maybe the tests could be even nicer but this is definitely better than the current state and not obviously unmaintainable/total mess so I'd merge it. You can always come back and make a second PR to clean up or refactor the tests.

Should I merge or rebase upstream or leave this branch where it is?

I'd rebase which will fix the docs failure (I think). At least locally the docs in master build.

alimanfoo and others added 3 commits Dec 24, 2018
@djhoese djhoese force-pushed the djhoese:git-update-submodules-alimanfoo-20181224-merry-xmas branch from 8a8ae73 to ec6973c Apr 8, 2019
@djhoese

This comment has been minimized.

Copy link
Contributor Author

djhoese commented Apr 8, 2019

Ok I rebased with master. I also made it so the submodule repository has two commits, but the parent repository wants the submodule at commit 1. This makes sure that our operations don't accidentally update the commit the submodule is at.

@betatim

This comment has been minimized.

Copy link
Member

betatim commented Apr 8, 2019

Merging! Thanks for picking this up and contributing the test.

@betatim betatim merged commit f3c7bf7 into jupyter:master Apr 8, 2019
4 of 5 checks passed
4 of 5 checks passed
codecov/project 90.72% (-0.08%) compared to 8d8c21e
Details
ci/circleci: build_docs Your tests passed on CircleCI!
Details
ci/dockercloud Your tests passed in Docker Cloud
Details
codecov/patch 50% of diff hit (target 20%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@djhoese djhoese deleted the djhoese:git-update-submodules-alimanfoo-20181224-merry-xmas branch Apr 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.