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

Splitting large tests #2090

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Splitting large tests #2090

wants to merge 5 commits into from

Conversation

noisekit
Copy link
Contributor

@noisekit noisekit commented Apr 4, 2024

No description provided.

@noisekit noisekit self-assigned this Apr 4, 2024
@noisekit noisekit force-pushed the Fix-failing-tests branch 3 times, most recently from b62d973 to 0954296 Compare April 11, 2024 07:53
@noisekit noisekit marked this pull request as ready for review April 11, 2024 08:10
@noisekit noisekit force-pushed the Fix-failing-tests branch 2 times, most recently from feed928 to 66c4419 Compare April 19, 2024 01:59
@noisekit noisekit changed the title Fix failing tests Splitting large tests Apr 22, 2024
@noisekit noisekit requested review from davidvuong and 0xjocke April 22, 2024 02:59
@noisekit noisekit force-pushed the Fix-failing-tests branch from 66c4419 to 0709de2 Compare April 22, 2024 03:02
@noisekit noisekit changed the base branch from main to Rerun-failed-tests-only April 22, 2024 03:02
Base automatically changed from Rerun-failed-tests-only to main April 22, 2024 04:03
@noisekit noisekit force-pushed the Fix-failing-tests branch 2 times, most recently from 9110e50 to dc75077 Compare April 23, 2024 00:43
Copy link
Contributor

Choose a reason for hiding this comment

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

Splitting it in 3 files that do the same looks wrong from code sharing... just a couple of params change on each file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know but this is a simplest step to take - split and copy. Any additional abstractions can be done later if they are necessary. Personally I do believe copying is much better for tests than abstracting things. Tests are write and forget for the most parts and this is not an actively refactored piece of code that has to be always same across all tests.


const MARKET_FEATURE_FLAG = ethers.utils.formatBytes32String('registerMarket');

describe('IssueUSDModule', function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

each shold have a different name to ease logs reading and isolation test runs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah good catch, I've missed this one here

@noisekit noisekit force-pushed the Fix-failing-tests branch from dc75077 to 3c56813 Compare April 23, 2024 04:17
@noisekit noisekit force-pushed the Fix-failing-tests branch from 3c56813 to 0e68fe2 Compare April 27, 2024 03:51
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