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

fix: txtar tests load full config and execute serially #1342

Merged

Conversation

deelawn
Copy link
Contributor

@deelawn deelawn commented Nov 7, 2023

This fixes an issue where executing txtar tests would fail because the example packages weren't loaded into memory.

Contributors' checklist...
  • Added new tests, or not needed, or not feasible
  • Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
  • Updated the official documentation or not needed
  • No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
  • Added references to related issues and PRs
  • Provided any useful hints for running manual tests
  • Added new benchmarks to generated graphs, if any. More info here.

@deelawn deelawn requested a review from a team as a code owner November 7, 2023 21:29
@deelawn deelawn requested a review from gfanton November 7, 2023 21:29
@github-actions github-actions bot added the 📦 ⛰️ gno.land Issues or PRs gno.land package related label Nov 7, 2023
Copy link
Member

@gfanton gfanton left a comment

Choose a reason for hiding this comment

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

thanks! fix a regression introduced by #1241

Copy link

codecov bot commented Nov 7, 2023

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (041d0a8) 55.90% compared to head (e8c4eb4) 55.96%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1342      +/-   ##
==========================================
+ Coverage   55.90%   55.96%   +0.05%     
==========================================
  Files         420      420              
  Lines       65373    65380       +7     
==========================================
+ Hits        36549    36592      +43     
+ Misses      25968    25931      -37     
- Partials     2856     2857       +1     
Files Coverage Δ
gno.land/pkg/integration/testing_integration.go 79.89% <100.00%> (+0.88%) ⬆️
gno.land/pkg/gnoland/node_inmemory.go 0.00% <0.00%> (ø)

... and 4 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@deelawn deelawn marked this pull request as draft November 8, 2023 00:03
@deelawn
Copy link
Contributor Author

deelawn commented Nov 8, 2023

I moved it back to draft status. There is an issue where running multiple txtar tests (see txtar test added in this PR) results in one of them always failing with the error: Invalid tx [error internal error log recovered: unexpected node with location gno.land/r/foobar/bar/:0. Replace the package path with the package path of the contract of the test that failed.

@moul
Copy link
Member

moul commented Nov 8, 2023

Yes, Txtar enforces for running tasks at the same time.

We need to either improve how Gnoland handles concurrency or change Txtar to run tasks one after the other.

@gfanton
Copy link
Member

gfanton commented Nov 8, 2023

If it's really a memory problem, another good solution would be to build & exec gnoland the same way we do with gno so it will have its own process, and no more memory conflict.

Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>
Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>
Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>
@deelawn deelawn marked this pull request as ready for review November 8, 2023 16:24
@deelawn deelawn changed the title fix: load full node config for integration testing fix: txtar tests load full config and execute serially Nov 8, 2023
@deelawn deelawn merged commit c33c9f2 into gnolang:master Nov 8, 2023
180 checks passed
@deelawn deelawn deleted the fix/integration-load-full-node-config branch November 8, 2023 16:29
moul pushed a commit to moul/gno that referenced this pull request Nov 14, 2023
This fixes an issue where executing txtar tests would fail because the
example packages weren't loaded into memory.

<details><summary>Contributors' checklist...</summary>

- [x] Added new tests, or not needed, or not feasible
- [ ] Provided an example (e.g. screenshot) to aid review or the PR is
self-explanatory
- [ ] Updated the official documentation or not needed
- [ ] No breaking changes were made, or a `BREAKING CHANGE: xxx` message
was included in the description
- [ ] Added references to related issues and PRs
- [ ] Provided any useful hints for running manual tests
- [ ] Added new benchmarks to [generated
graphs](https://gnoland.github.io/benchmarks), if any. More info
[here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md).
</details>

---------

Signed-off-by: gfanton <8671905+gfanton@users.noreply.github.com>
Co-authored-by: gfanton <8671905+gfanton@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 ⛰️ gno.land Issues or PRs gno.land package related
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants