Skip to content

Test Optimization Proposal (SC-736)#1188

Merged
TheRealFalcon merged 5 commits into
canonical:mainfrom
holmanb:holmanb/test-optimization
Jan 14, 2022
Merged

Test Optimization Proposal (SC-736)#1188
TheRealFalcon merged 5 commits into
canonical:mainfrom
holmanb:holmanb/test-optimization

Conversation

@holmanb
Copy link
Copy Markdown
Member

@holmanb holmanb commented Jan 13, 2022

Reduce template rendering test runtime

Additional Context

Pytest consistently shows the parametrized template rendering tests as the most time-consuming in the unit test suite (0.5s per test). Profile analysis shows that nearly all of the time spent is in one-time setup tasks (importing modules). Currently each test invokes a subprocess to execute the test.

This change should be non-functional. It simply moves the rendering code into the templater module and uses function calls in place of spawning a new process per parametrized test (function calls share the setup cost of module import). A single test that calls the tool as a subprocess was added to maintain test coverage.

This saves ~7s on my machine (which is 13% of the total unittest runtime).

@holmanb holmanb changed the title Test Optimization Proposal Test Optimization Proposal (SC-736) Jan 13, 2022
Copy link
Copy Markdown
Contributor

@TheRealFalcon TheRealFalcon left a comment

Choose a reason for hiding this comment

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

Good idea. These tests have bugged me in the past too.

My only concern is when it comes to separating concerns, I'm not a big fan of having functions that combine taking cli/argparse args with additional logic, because those arguments can pretty hard to construct outside of calling from the CLI. That makes reuse and testing harder.

Can we instead have the render_cloudcfg function take arguments like variant, template, and output_file (or whatever), and leave all of the argument parsing where it was?

@holmanb
Copy link
Copy Markdown
Member Author

holmanb commented Jan 13, 2022

Good feedback, I agree. Fixed in the latest commit.

Copy link
Copy Markdown
Contributor

@TheRealFalcon TheRealFalcon 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, thanks!

@TheRealFalcon TheRealFalcon merged commit 73b1bb1 into canonical:main Jan 14, 2022
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