Clean process_inbox entries from the end-to-end tests
#2228
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MathieuDutSik
force-pushed
the
add_process_inbox
branch
from
c8636cb
to
971b564
Compare
afck
reviewed
Jul 8, 2024
| @@ -1065,6 +1065,8 @@ async fn test_wasm_end_to_end_non_fungible(config: impl LineraNetConfig) -> Resu | |||
| ) | |||
| .await; | |||
|
|
|||
| // The process inbox is needed on chain1 in order to get the application. | |||
| node_service1.process_inbox(&chain1).await?; | |||
There was a problem hiding this comment.
If the transfer call above succeeded, it must already have the application. This process_inbox really shouldn't be necessary, as discussed.
There was a problem hiding this comment.
Ok, let me revert the commit (The process_inbox had been removed but reintroduced after the CI failure).
There was a problem hiding this comment.
(If removing this really breaks CI, we should keep it, of course, and create an issue to track the bug.)
There was a problem hiding this comment.
Ok, will see. I need to be convinced that the removal are triggering that problem and I am not convinced so far.
MathieuDutSik
force-pushed
the
add_process_inbox
branch
from
7f35696
to
4649182
Compare
MathieuDutSik
force-pushed
the
add_process_inbox
branch
from
4649182
to
4ba8768
Compare
|
The PR is now a little old and not the right approach. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
The CI is progressing in quality, but it still has some issues. This PR contains a number of corrections.
Proposal
The following is done:
with_contextinwallet.rsis reduced to the first 200 which makes the logs easier to analyze.test_open_chain_node_serviceis eliminated and replaced by aprocess_inbox.process_inboxis removed for thecanceloperations in thematching_enginesince it is not needed.process_inboxin thetest_wasm_end_to_end_non_fungibleare not needed because in atransferoperation, only the target chain needs to have aprocess_inbox.Test Plan
The CI is of course the objective. But the methodology for checking was different. Tests were run 10 times locally in order to identify if a problem was introduced by the removal of
process_inbox. The result showed that no errors were introduced by doing that. It does not mean that there are no remaining errors.Release Plan
A priori is not relevant, but if
process_inboxwill continue to to exist in the future, then we need to document how to use it.Links