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

CircleCI: parameterizing no_output_timeout and -short #2618

Merged
merged 7 commits into from
Jul 28, 2021
Merged

CircleCI: parameterizing no_output_timeout and -short #2618

merged 7 commits into from
Jul 28, 2021

Conversation

algobarb
Copy link
Contributor

@algobarb algobarb commented Jul 23, 2021

Summary

Parameterize no_output_timeout and -short in the general commands on the circle config file to reduce duplicate code.

I did also increase the size of integration tests to large as I saw that we had errors in mac_amd64_integration, arm64_integration and arm64_integration recently but would run after manually rerunning from failure.

Test Plan

Tested it on a Pull request that would run the nightly and then tested the regular short tests once.

@algobarb algobarb self-assigned this Jul 23, 2021
@codecov-commenter
Copy link

codecov-commenter commented Jul 23, 2021

Codecov Report

Merging #2618 (569c011) into master (099cdd2) will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2618      +/-   ##
==========================================
- Coverage   46.98%   46.97%   -0.01%     
==========================================
  Files         348      348              
  Lines       55717    55717              
==========================================
- Hits        26179    26174       -5     
- Misses      26594    26601       +7     
+ Partials     2944     2942       -2     
Impacted Files Coverage Δ
ledger/blockqueue.go 82.18% <0.00%> (-2.88%) ⬇️
network/requestTracker.go 70.25% <0.00%> (-0.87%) ⬇️
network/wsPeer.go 74.37% <0.00%> (-0.84%) ⬇️
ledger/acctupdates.go 61.88% <0.00%> (-0.34%) ⬇️
data/transactions/verify/txn.go 48.45% <0.00%> (ø)
cmd/tealdbg/debugger.go 73.86% <0.00%> (+1.00%) ⬆️
crypto/merkletrie/node.go 93.48% <0.00%> (+1.86%) ⬆️
crypto/merkletrie/trie.go 68.61% <0.00%> (+2.18%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 099cdd2...569c011. Read the comment docs.

@algobarb algobarb changed the title [CircleCI TEST] test parameterizing no_output_timeout and -short CircleCI: test parameterizing no_output_timeout and -short Jul 24, 2021
@algobarb algobarb requested a review from cce July 24, 2021 00:41
@algobarb algobarb marked this pull request as ready for review July 24, 2021 00:41
Copy link
Contributor

@bricerisingalgorand bricerisingalgorand left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@egieseke egieseke left a comment

Choose a reason for hiding this comment

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

Looks good.

default: "/home/circleci"
result_subdir:
default: 30m
short_test_flag:
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this could be made more generic to support any test flag?

Copy link
Contributor Author

@algobarb algobarb Jul 27, 2021

Choose a reason for hiding this comment

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

I'm not sure I want to do that anymore:

  1. We're calling a bunch of other parameters inside the test itself right now, so it could be confusing to add an any_test flag because you can't add whatever parameter you want. Only parameters that haven't been specified yet
  2. -short is the main flag we're using and it helps to repeat the same variable name used in generic_integration for consistency
  3. Will mentioned that he wants us to stop calling gotestsum directly in the file eventually, so I expect further modifications on this command. (1426 in our CircleCI epic)

Copy link
Contributor

@bricerisingalgorand bricerisingalgorand left a comment

Choose a reason for hiding this comment

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

LGTM

@algobarb algobarb changed the title CircleCI: test parameterizing no_output_timeout and -short CircleCI: parameterizing no_output_timeout and -short Jul 27, 2021
@algojohnlee algojohnlee merged commit 434b98f into algorand:master Jul 28, 2021
@algobarb algobarb deleted the barbara/circleaddparams branch July 28, 2021 13:40
algojack pushed a commit that referenced this pull request Jul 29, 2021
Parameterize no_output_timeout and -short in the general commands on the circle config file to reduce duplicate code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants