testnode: Allow to fetch submodule when running testnode
-
added 20 commits
-
20d4c001...80994259 - 17 commits from branch
nexedi:master - 0cbe60a1 - Update submodule when running testnode
- 77a84720 - testnode: Also reset the submodule when rollback the main module
- ea8d3705 - testnode: Add submodule check when checking revision
Toggle commit list -
20d4c001...80994259 - 17 commits from branch
-
added 12 commits
-
205ec0cd...3f91377e - 9 commits from branch
nexedi:master - b96560df - testnode: Update repo's submodule when running testnode
- 9b9bdaaa - testnode: Also reset the submodule when rollback the main module
- 3abd7b45 - testnode: Add submodule check when checking revision
Toggle commit list -
205ec0cd...3f91377e - 9 commits from branch
-
With your changes testERP5TypeNode fails with :
OSError: [Errno 2] No such file or directory: '/tmp/tmp9cyiNZ/testnode/foo/rep0'Also, I strongly disagree to change test_05_updateRevisionList. With your change, we don't know if getAndUpdateFullRevisionList is working without submodules. Also you only check that self.assertNotEquals(submodule_revision0, submodule_revision1).... while this would be the case even without submodules, right ?
So I would prefer that you check the content of a file within the submodule, this would be a better proof. And this should go in a separate test.
Also I would like also the proof that submodules are correctly reset, like we have for : https://lab.nexedi.com/nexedi/erp5/blob/d28d8f2d53abf84ad701042041e76f8196e79846/erp5/tests/testERP5TestNode.py#L472 . Again, checking the content of a file in the submodule in a separate test would be nice.
-
@seb Hi, I updated the test file.
- I am using a separated test to check the submodule.
- The original test was trying to check the revision in the submodule repository itself(
temp_dir/repo2), which as you said, is unreliable. In the new test, I check the revision in the repo0's submodule dir(temp_dir/repo1/repo2). Which has the same effect as check the file content. - The error you mentioned was fixed. It is because we should not update submodule in a corrupt repo.
-
erp5/tests/testERP5TestNode.py
499 # Get the current submodule revision 500 repo1_repo2 = os.path.join(self.remote_repository1, 'repo2') 501 call = self.getCaller(cwd=repo1_repo2) 502 submodule_revision0 = call("git rev-parse HEAD".split()) 503 504 # Add new commit to submodule 505 call = self.getCaller(cwd=self.remote_repository2) 506 submodule_commit_file = open(os.path.join(self.remote_repository2, 'first_file'), 'w') 507 submodule_commit_file.write("next_content") 508 submodule_commit_file.close() 509 call("git commit -av -m new_commit".split()) 510 511 # Update the submodule in the main repo 512 call = self.getCaller(cwd=repo1_repo2) 513 call("git pull".split()) 514 submodule_revision1 = call("git rev-parse HEAD".split()) -
But here you do yourself the git pull... So, except if I missed something, your test check that git works, but not getAndUpdateFullRevisionList.
-
This step means we did something in the submodule. Let's start from beginning:
First
repo2/file_two (content : bb, sha : abcd1234) repo1/file_one (content : aa) repo1/repo2/ (point to sha : abcd1234) repo1/repo2/file_two (content : bb)submodule_revision0isabcd1234.Then I add a new commit in repo2:
repo2/file_two (content : bbcc, sha : efgh5678) repo1/file_one (content : aa) repo1/repo2/ (point to sha : abcd1234) repo1/repo2/file_two (content : bb)Then this step, aka
git pullwas done inrepo1/repo2:repo2/file_two (content : bbcc, sha : efgh5678) repo1/file_one (content : aa) repo1/repo2/ (In repo1, it point to sha : abcd1234, but now the content point to efgh5678) repo1/repo2/file_two (content : bbcc, sha : efgh5678)submodule_revision1isefgh5678The "rollback" functionality should keep the content of
repo1/repo2to the shaabcd1234which repo1 noted, right?So after
getAndUpdateFullRevisionList. The test gets the commit sha inrepo1/repo2, aka `submodule_revision2. It should be restored toabcd1234. -
There is surely a misunderstanding somewhere.
updateRevisionList (https://lab.nexedi.com/nexedi/erp5/blob/d45c6f7cbf3b01e361961bed927c0adae5b2fead/erp5/util/testnode/testnode.py#L155) is using an Updater without passing revision parameter, therefore we should get latest available revision without rollback.
the rollback functionality is used by checkRevision (https://lab.nexedi.com/nexedi/erp5/blob/d45c6f7cbf3b01e361961bed927c0adae5b2fead/erp5/util/testnode/testnode.py#L196) which is using Updater with a revision parameter.
So in your comment here you talk about rollback while you are checking method updateRevisionList.
So please fully comment in your test expected status of remote and local repositories (like you are greatly doing in your comment just above). And come back to me if either I missed something or if you would like further explanations.
-
-
In general, you should probably commit more the unit test, to make it clear what should be updated.
Also, I already wrote that I would prefer that you check the content of files. Like :
Before :
repo1/file_one (content : aa) repo1/submodule/file_two (content : bb)You do some commits in remote repo (to change content of file_one to aacc and content of file_two to bbdd), and you check that after getAndUpdateRevisionList you have what is expected:
After:
repo1/file_one (content : aacc) repo1/submodule/file_two (content : bbdd)Do you see what I mean ?
-
I will add this logic.
-
erp5/tests/testERP5TestNode.py
492 572 return info_list 493 573 self.assertEquals(['2', '2'], getRepInfo(count=1)) 494 574 self.assertEquals([commit_dict['rep0'][0][0],commit_dict['rep1'][0][0]], 575 getRepInfo(hash=1)) -
Looks like you changed the indentation of that line by mistake. Then in the diff it looks like you changed test_06_checkRevision, while in reality you just change the indentation of one line. Please fix this commit to not change test_06_checkRevisision at all.
-
-
erp5/tests/testERP5TestNode.py
608 if hash: 609 info_list.append( 610 call("git log -n1 --format=%H".split()).strip()) 611 return info_list 612 # Include the submodule commit in rep1 which added in setUpSubmodules 613 self.assertEquals(['2', '3'], getRepInfo(count=1)) 614 # The latest revision in rep1 should be the submodule commit revision 615 self.assertEquals([commit_dict['rep0'][0][0],rep1_revision_0], 616 getRepInfo(hash=1)) 617 618 # Add something in cloned repository rep1/rep2 619 rep1_clone_path = [x['repository_path'] for x in \ 620 node_test_suite.vcs_repository_list \ 621 if x['repository_path'].endswith("rep1")][0] 622 rep1_clone_rep2_path = os.path.join(rep1_clone_path, "rep2") 623 call = self.getCaller(cwd=rep1_clone_rep2_path) -
This is quite unrealistic. This is somehow like you simulate someone doing changes in the clone repository. But this should not be the case, and I do not like testing this scenario.
And this should not be needed, since later in the test, you force test_result.revision_list to go back to a previous status.
-
-
632 call("git commit -av -m new_commit".split()) 633 634 # Get new revision in rep1/rep2 635 clone_rep1_rep2_revision_1 = call("git rev-parse HEAD".split()).rstrip() 636 637 # The rep1/rep2 revision was changed 638 self.assertNotEquals(clone_rep1_rep2_revision_0, clone_rep1_rep2_revision_1) 639 640 class TestResult(object): 641 revision = NodeTestSuite.revision 642 test_result = TestResult() 643 # for test result to be one commit late for rep1 to force testnode to 644 # reset tree to older version 645 test_result.revision_list = (('rep0', (2, commit_dict['rep0'][0][0])), 646 ('rep1', (1, commit_dict['rep1'][1][0]))) 647 test_node.checkRevision(test_result, node_test_suite) -
Here this fine. You force testnode to reset repositories to a previous status.
-
-
added 32 commits
-
83130ee0...f766e714 - 30 commits from branch
nexedi:master - edc906af - testnode: Update repo's submodule when running testnode
- a94325c4 - testnode: Add submodule check tests when checking revision
Toggle commit list -
83130ee0...f766e714 - 30 commits from branch
-
erp5/util/testnode/Updater.py
164 164 logger.exception("") 165 165 if not(correct_url): 166 166 self.deleteRepository() 167 else: 168 # Get the submodule only if we are in the correct repo 169 try: 170 parameter_list = ['submodule', 'update', '--init'] 171 self._git(*parameter_list) 172 except SubprocessError: 173 # If working on a corrupt repository, just ignore it 174 logger.exception("") -
I didn't checked carefully theses subprocess error up to know. What logger.exception("") is doing ? just nothing ? something in log ?
If a repository is corrupted, then testnode must automatically fix it. If we don't do so, then from time to time we will have corrupted repositories and we would need to do something manually. And we really want testing infrastructure running as much as possible without any human action.
For example see : https://lab.nexedi.com/nexedi/erp5/blob/3baa2992f3b5ab9c1f512241a559db737b7ba38c/erp5/util/testnode/testnode.py#L170 .
So if you know cases where repositories are corrupted due to submodules, we should have the case simulated in unit test and having testnode fixing it alone.
-
This is for the test
test_05f_ResetCorruptedRepository. This test will truncate the repository on purpose. So the submodule command failed. But even submodule command failing was suppressed, other following git commands still going to fail and reports an error. Then the testnode fix the repo automatically.And you are correct, I shouldn't suppress the submodule error. But due to other commands will fail eventually, so the result has no impact. Now it was removed.
-
-
Both commits should be merged (so that we see directly how unit test cover changes). Could you please merge the two commits together. My only concern now is my comment about hidden exceptions.
-
closed
Toggle commit list
-
Please register or sign in to post a comment