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

Re-enable running tests via IR in external tests in cases where they don't pass due to Hardhat heuristics #12736

Merged
merged 1 commit into from
Mar 11, 2022

Conversation

cameel
Copy link
Member

@cameel cameel commented Mar 3, 2022

Depends on #12195. Merged.

So far only compilation was enabled in IR presets for OpenZeppelin, PRBMath, ElementFi and (after #12197) Gnosis. This is due to NomicFoundation/hardhat#2115, which is still unresolved. Unfortunately this does not give us any benchmarks via IR for these projects. In this PR I'm re-enabling tests and instead disabling individual test cases that do not pass.

The PR also disables one particularly flaky test file in ElementFi. It has one case that fails often and has been reported upstream (delvtech/elf-contracts#240) but also many tests in the file seem to depend on an external service, which has resulted in a mass failure due to network timeout at least once when the service was down. EDIT: This part moved to #12764.

@cameel cameel force-pushed the reenable-tests-via-ir-in-ext-tests branch from 9a135e8 to e4b2977 Compare March 3, 2022 17:56
@cameel cameel changed the base branch from develop to update-gnosis-ext-test March 3, 2022 17:56
@cameel cameel added the has dependencies The PR depends on other PRs that must be merged first label Mar 3, 2022
@cameel cameel self-assigned this Mar 3, 2022
@cameel cameel marked this pull request as draft March 3, 2022 23:27
@cameel cameel force-pushed the reenable-tests-via-ir-in-ext-tests branch from e4b2977 to 4f6815c Compare March 4, 2022 13:05
@cameel
Copy link
Member Author

cameel commented Mar 4, 2022

There are 15 failures for ElementFi via IR unrelated to NomicFoundation/hardhat#2115 (see job 994880):

  1. 2 cases of address not matching expectations.
  2. 5 cases of a revert with wrong reason string.
  3. 8 cases of unexpected reverts.
    • Could be related to (2). Still investigating this one.

@cameel cameel force-pushed the update-gnosis-ext-test branch from b9bde34 to c8803e6 Compare March 4, 2022 17:41
@cameel cameel force-pushed the reenable-tests-via-ir-in-ext-tests branch from 4f6815c to 4e7ef7c Compare March 4, 2022 17:41
@cameel
Copy link
Member Author

cameel commented Mar 4, 2022

Turns out that all 15 failures were caused by #12738. The reverts were caused by external calls to wrongly calculated address and that's why they did not have a revert message. The issue I found in Hardhat is still real but it's a different thing that I just happened to find by accident.

@cameel cameel force-pushed the reenable-tests-via-ir-in-ext-tests branch from 4e7ef7c to cca9c79 Compare March 4, 2022 20:22
@cameel
Copy link
Member Author

cameel commented Mar 4, 2022

The failure in OZ test seems to be just a case of NomicFoundation/hardhat#2453 triggered under more specific conditions (see NomicFoundation/hardhat#2453 (comment)).

@ekpyron's #12744 fixes the failures on ElementFi so I won't be adding workarounds for them in this PR. It does not fix the OZ one though so I'm disabling this test case in OZ. With that, all tests should start passing here once #12744 is merged.

@cameel cameel force-pushed the reenable-tests-via-ir-in-ext-tests branch from d36ca43 to 7404526 Compare March 9, 2022 17:58
@cameel cameel force-pushed the update-gnosis-ext-test branch 2 times, most recently from 11a8c49 to 438c082 Compare March 9, 2022 18:57
@cameel cameel force-pushed the reenable-tests-via-ir-in-ext-tests branch from 7404526 to 1ddca4d Compare March 9, 2022 18:57
Base automatically changed from update-gnosis-ext-test to develop March 10, 2022 07:42
@ekpyron
Copy link
Member

ekpyron commented Mar 10, 2022

Looks like this has a merge conflict now - but apart from that there's nothing blocking this anymore, is there?

@cameel cameel force-pushed the reenable-tests-via-ir-in-ext-tests branch from 1ddca4d to e9f3f93 Compare March 10, 2022 13:13
@cameel
Copy link
Member Author

cameel commented Mar 10, 2022

Yeah, a small conflict with #12764. They both add some new lines at the same spot.

Conflict resolved.

@cameel
Copy link
Member Author

cameel commented Mar 10, 2022

but apart from that there's nothing blocking this anymore, is there?

It's ready unless it turns out that some tests are still failing.

@cameel cameel removed the has dependencies The PR depends on other PRs that must be merged first label Mar 10, 2022
Copy link
Member

@ekpyron ekpyron left a comment

Choose a reason for hiding this comment

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

The failing euler test is unrelated and hopefully fixed by #12768 (which I just merged).

@ekpyron ekpyron marked this pull request as ready for review March 11, 2022 12:44
@ekpyron
Copy link
Member

ekpyron commented Mar 11, 2022

I'll just go ahead and un-draft and merge this.

@ekpyron ekpyron merged commit 2696377 into develop Mar 11, 2022
@ekpyron ekpyron deleted the reenable-tests-via-ir-in-ext-tests branch March 11, 2022 12:44
@cameel
Copy link
Member Author

cameel commented Mar 11, 2022

Ah, right. Forgot to undraft it. Thanks!

And yeah, euler test failure was unrelated, should be fixed by #12768.

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.

2 participants