The Wayback Machine - https://web.archive.org/web/20220306190655/https://github.com/ethereum/solidity/pull/12634
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

Jump to bottom

Remove a reference to unused SolidityFixedFeeRegistrar #12634

Merged
merged 1 commit into from Feb 7, 2022
Merged

Remove a reference to unused SolidityFixedFeeRegistrar #12634

merged 1 commit into from Feb 7, 2022

Conversation

a3d4
Copy link
Contributor

@a3d4 a3d4 commented Feb 5, 2022

This fixes soltest on Windows in Debug configuration.
@cameel cameel changed the base branch from develop to hardhat-on-nodejs-16 Feb 6, 2022
cameel
cameel reviewed Feb 6, 2022
View changes
test/boostTest.cpp
@@ -279,7 +279,6 @@ test_suite* init_unit_test_suite( int /*argc*/, char* /*argv*/[] )
"ABIDecoderTest",
"ABIEncoderTest",
"SolidityAuctionRegistrar",
"SolidityFixedFeeRegistrar",
Copy link
Member

@cameel cameel Feb 6, 2022 •
edited

Choose a reason for hiding this comment

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

We still have it under externalTests/FixedFeeRegistrar. Not sure why these suites are disabled though (i.e. failures, slowdowns, or maybe they already run separately?) so I'm not sure if we should update the path or remove it.

BTW, what's the problem with it on Windows? What are we missing that it does not fail in CI?
Copy link
Member

@cameel cameel Feb 6, 2022

Choose a reason for hiding this comment

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

Ah, wait these reference the suites from .cpp files and in #11426 @axic moved this one to a .sol file. So regardless of why these suites are ignored, SolidityFixedFeeRegistrar is indeed not needed here.
Copy link
Contributor Author

@a3d4 a3d4 Feb 6, 2022

Choose a reason for hiding this comment

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

what's the problem with it on Windows

The problem was in Debug configuration (Windows should not really be relevant indeed), because of the failing assert.

Maybe I should put it a way around, and say that the problem was that tests were succeeding in non-Debug configurations because assert was disabled there :-).
Copy link
Member

@cameel cameel Feb 7, 2022

Choose a reason for hiding this comment

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

Oh, really? We should definitely have plain asserts enabled, even if we generally avoid them. That can be done separately though: #12641.
cameel
cameel approved these changes Feb 6, 2022
View changes
@cameel cameel mentioned this pull request Feb 7, 2022
Open
@cameel cameel merged commit 175c130 into ethereum:hardhat-on-nodejs-16 Feb 7, 2022
55 checks passed
@cameel cameel mentioned this pull request Feb 7, 2022
Merged
@cameel
Copy link
Member

@cameel cameel commented Feb 7, 2022 •
edited

I mistakenly merged it before merging the base PR so it went into the wrong branch. Reopened as #12642.
@cameel cameel mentioned this pull request Feb 7, 2022
Merged
@a3d4 a3d4 mentioned this pull request Feb 8, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has dependencies testing 🔨
2 participants
@a3d4 @cameel