[ISSUE 461] Fix issue when getting the fork name for the cases where the fork has a different name #462
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -158,13 +158,22 @@ test("PR from owner1/target:branchA to owner2/target:branchB while using mapping | |
| mockApi: [ | ||
| moctokit.rest.repos | ||
| .get({ | ||
|
There was a problem hiding this comment. Ok this seems like it was a hack to get around the issue that we're fixing (to get into the other path of the getForkName method). Seems valid not to fail resolution of the repos. There was a problem hiding this comment. we already have a successful resolution of owner2/project4 in the block after this one. In this cross-pr.test.ts, Owner1 has project1, project2, project3 and project4 while Owner 2 has only project3 and project4, that's why I mocked project1 and 2 to fail state and project 3 and 4 to ok state. There was a problem hiding this comment. I was talking about owner1/project4 , it should not affect how the test runs now, but it might "strengthen" the test context ... anyway, just a minor note really. There was a problem hiding this comment. @jstastny-cz I would go without is for now, as AFAIK we are not doing it for the other tests like full-downstream.test.ts, otherwise I think we would need to apply this everywhere to be consistent (even not affecting the test result) There was a problem hiding this comment. @jstastny-cz if you are ok with the PR state, please approve it |
||
| owner: "owner2", | ||
| repo: /project(1|2)/, | ||
| }) | ||
| .setResponse({ | ||
| status: 404, | ||
| data: {}, | ||
| repeat: 2 | ||
| }), | ||
| moctokit.rest.repos | ||
| .get({ | ||
| owner: "owner2", | ||
| repo: "project4" | ||
| }) | ||
| .setResponse({ | ||
| status: 200, | ||
| data: {}, | ||
| }), | ||
| moctokit.rest.repos | ||
| .listForks({ | ||
|
|
@@ -184,12 +193,12 @@ test("PR from owner1/target:branchA to owner2/target:branchB while using mapping | |
| }), | ||
| moctokit.rest.repos | ||
| .get({ | ||
| owner: "owner2", | ||
rgdoliveira marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| repo: "project3", | ||
| }) | ||
| .setResponse({ | ||
| status: 200, | ||
| data: { name: "project3", owner: { login: "owner2" } }, | ||
| }), | ||
| moctokit.rest.repos | ||
| .listForks({ | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jstastny-cz @Ginxo TBH I'm not sure if using
checkIfRepositoryExistswithsourceOwnerinstead oftargetOwneris the correct way to go, after this change I'm getting many failures in the e2e tests locally and seems that in GH they are running forever and never finish.To run them locally I used:
Actually taking a look in the existing single-pr.test.ts e2e test, there is test
PR from owner2/target:branchA to owner1/target-different-name:branchBwhich should be testing the case where the fork has a different name from the original repository and the test is passing. Am I missing something?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's it, this functionality is already covered and it shouldn't be re-adapted unless we discover specific corner case for it I'm not able to find (can't get trace logs).
I'm afraid we are missing something on the BC configuration rather than BC bug, but it is just a feeling I would like to confirm/discard. Could you please provide logs URL? Many thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it really seem correct to you @Ginxo ? We want to check here in the method if there's a fork with the same name as repo name ... but why would we check that the repo in target exists as the first thing and if so then return?
In code we invoke
getForkName(currentTarget.group, originalSource.group!, currentTarget.name)in checkout-service.ts:L163, where the currentTarget variable in same file L140 is commented asThat really sounds like identity check. And the single-pr.test.ts - I am gonna check, but in this case I have mutliple PRs across 3 repositories, maybe it tests a different thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It certainly does not do what the comment just above it says
// check whether there is a fork with the same name as repo nameThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider this reproducer :
getForkName returns "repo".