Skip to content

GitHubRepositoryForks: Update Repo Name in Tests #251

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

Merged
merged 1 commit into from
Jun 28, 2020

Conversation

X-Guardian
Copy link
Contributor

@X-Guardian X-Guardian commented Jun 28, 2020

Description

This PR updates the upstream Owner and Repo name used in the GitHubRepositoryForks tests to cause less destruction.

Issues Fixed

None

References

N/A

Checklist

  • You actually ran the code that you just wrote, especially if you did just "one last quick change".
  • Comment-based help added/updated, including examples.
  • Static analysis is reporting back clean.
  • New/changed code adheres to our coding guidelines.
  • New/changed code continues to support the pipeline.
  • Changes to the manifest file follow the manifest guidance.
  • Unit tests were added/updated and are all passing. See testing guidelines. This includes making sure that all pipeline input variations have been covered.
  • Relevant usage examples have been added/updated in USAGE.md.
  • If desired, ensure your name is added to our Contributors list

@HowardWolosky
Copy link
Contributor

I merged this in immediately yesterday without bothering with verifying CI because the change seemed so banal. In practice though, it looks like this has somehow almost doubled the runtime of the CI.

Up until this point, builds had been running in the 30-35 min range. I re-ran a CI build this morning at 41de3ad (that's the commit in master right before this one), and it completed in 35 min. I'm running another one now for verification.

The very next commit is 19c8417 (this change). That build is taking about 60 min (the build times out if any job exceeds that time). Every CI build that has happened after that commit went in is now running at around 60 min.

I'm inclined to roll this change back until the change is better understood. When I run GitHubRepositoryForks.tests.ps1 locally with this change, it only takes 2.5 min, so it's not clear to me why this is so much more negatively impacting the CI build than local builds.

A possible middle ground would be to to define different values for upstreamOwnerName and upstreamRepositoryName depending on if the test is running in CI or locally. But I'd still like to better understand the big difference in runtime behavior with this single change.

@X-Guardian
Copy link
Contributor Author

X-Guardian commented Jun 29, 2020

You are right, it is taking 20 minutes in the CI to run the fork tests. Here is a snippet of the raw logs from one of the CIs with times:

2020-06-28T16:04:39.0003728Z   Describing Creating a new fork for user
2020-06-28T16:04:39.0998198Z 
2020-06-28T16:04:39.1011924Z     Context When a new fork is created
2020-06-28T16:04:41.1599064Z WARNING: The server has indicated that the result is not yet ready (received status code of [202]).
2020-06-28T16:04:41.2180770Z WARNING: Forking a repository happens asynchronously.  You may have to wait a short period of time (up to 5 minutes) 
2020-06-28T16:04:41.2182044Z before you can access the git objects.
2020-06-28T16:11:09.3736488Z       [+] Should be in the list 43ms
2020-06-28T16:11:09.5293438Z       [+] Should have the expected additional type and properties 92ms
2020-06-28T16:11:10.2612043Z 
2020-06-28T16:11:10.2626272Z     Context When a new fork is created (with the pipeline)
2020-06-28T16:11:13.2449862Z WARNING: The server has indicated that the result is not yet ready (received status code of [202]).
2020-06-28T16:11:13.3030485Z WARNING: Forking a repository happens asynchronously.  You may have to wait a short period of time (up to 5 minutes) 
2020-06-28T16:11:13.3044831Z before you can access the git objects.
2020-06-28T16:17:40.8313500Z       [+] Should be in the list 43ms
2020-06-28T16:17:40.9802888Z       [+] Should have the expected additional type and properties 88ms
2020-06-28T16:17:41.8411993Z 
2020-06-28T16:17:41.8423801Z   Describing Creating a new fork for an org
2020-06-28T16:17:41.9596507Z 
2020-06-28T16:17:41.9610350Z     Context When a new fork is created
2020-06-28T16:17:44.3636775Z WARNING: The server has indicated that the result is not yet ready (received status code of [202]).
2020-06-28T16:17:44.4306263Z WARNING: Forking a repository happens asynchronously.  You may have to wait a short period of time (up to 5 minutes) 
2020-06-28T16:17:44.4307772Z before you can access the git objects.
2020-06-28T16:24:16.7644540Z       [+] Should be in the list 47ms

It is a tiny repo, with only one file, but it does already have 1,400 forks. I think running Get-GitHubRepositoryFork in the tests is taking the time. Try octocat/hello-world instead which only has 55 forks, or octocat/test-repo1 which only has 4 forks?

HowardWolosky added a commit to HowardWolosky/PowerShellForGitHub that referenced this pull request Jun 29, 2020
Pull microsoft#251 changed the repo targeted by the forks tests from
`microsoft/PowerShellForGitHub` to `octocat/Hello-World` in order
to prevent the accidental deletion of real forks when running the UT's
locally against your own account.

In practice, this almost doubled the execution time of the UT's,
because the execution time of the Forks API's took so much longer
against a repo with 1400+ forks.

Changing the test over to use a repo that currently only has 39 forks
(several orders of magnitude fewer).
HowardWolosky added a commit that referenced this pull request Jun 29, 2020
Pull #251 changed the repo targeted by the forks tests from `microsoft/PowerShellForGitHub` to `octocat/Hello-World` in order to prevent the accidental deletion of real forks when running the UT's locally against your own account.

In practice, this almost doubled the execution time of the UT's, because the execution time of the Forks API's took so much longer against a repo with 1400+ forks.

Changing the test over to use a repo that currently only has 39 forks (several orders of magnitude fewer).
@X-Guardian X-Guardian deleted the Forks-Tests-Change-Repo branch June 30, 2020 07:01
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.

2 participants