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

do not run commit hooks in timing tests #1151

Merged
merged 1 commit into from
Oct 1, 2021

Conversation

jtnord
Copy link
Member

@jtnord jtnord commented Oct 1, 2021

Do not run commit hooks in timing test

if you are mandated to have something like git secrets installed then the test fails as it takes more time that the test allows to make the commit.

On windows git secrets can easily make a commit take 20s to complete or more (but a lower bound is around 5 seconds).

As we are doing timing here and we do not care about commit hooks, we can just skip them so they do not interfere.

Checklist

  • I have read the CONTRIBUTING doc
  • Unit tests pass locally with my changes
  • I have added documentation as necessary
  • No Javadoc warnings were introduced with my changes
  • No spotbugs warnings were introduced with my changes
  • Documentation in README has been updated as necessary
  • Online help has been added and reviewed for any new or modified fields
  • Any dependent changes have been merged and published in upstream modules (like git-client-plugin)

Types of changes

  • Test

if you are mandated to have something like `git secrets` installed then
the test fails as it takes more time that the test allows to make the
commit.

On windows git secrets can easily make a commit take 20s to complete or
more (but a lower bound is around 5 seconds).

As we are doing timing here and we do not care about commit hooks, we
can just skip them so they do not interfere.
@@ -161,17 +161,17 @@ public void retrieveHeadsSupportsTagDiscovery_findTagsWithTagDiscoveryTrait() th
sampleRepo.init();
sampleRepo.git("checkout", "-b", "dev");
sampleRepo.write("file", "modified");
sampleRepo.git("commit", "--all", "--message=dev-commit-message");
sampleRepo.git("commit", "--all", "--message=dev-commit-message", "--no-verify");
long beforeLightweightTag = System.currentTimeMillis();
Copy link
Member Author

@jtnord jtnord Oct 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should also not use currentTimeMillis but nonoTime as the clock can change during testing (NTP step or drifting) causing flaky tests - but well that can be left for another day :)

Copy link
Contributor

@MarkEWaite MarkEWaite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@MarkEWaite MarkEWaite merged commit 41699e4 into jenkinsci:master Oct 1, 2021
@MarkEWaite MarkEWaite added hacktoberfest-accepted Accepted by maintainers for Hacktoberfest and removed hacktoberfest labels Oct 29, 2021
jtnord added a commit to jtnord/git-plugin that referenced this pull request Jan 18, 2022
Similar to jenkinsci#1151 by default git comes without any hooks, but when
creating a new sample git repo with the code it would copy the system
default hooks, meaning this sample repo can behave differently on
different platforms.

by not using the hooks here we can avoid littering test code with
"--no-verify" on commits, and can avoit hooks in other cases as well,
which consume test time and / or even fail the operation due to a
failing hook requirement (e.g. no JIRA reference)

when consuming this in pipeline-model-definition's ValidatorTest test
locally go from failing due to timeout exception (10 minutes) to
completing in 2 minutes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted Accepted by maintainers for Hacktoberfest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants