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

Remove CFG_USERINPUTFD and use stdin replacement in affected tests. #2091

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

Conversation

antonsviridenko
Copy link
Contributor

Closes #1609 and closes #1857

Affected tests now use stdin temporarily replaced with a pipe read end.

@codecov
Copy link

codecov bot commented Jun 14, 2023

Codecov Report

Patch coverage: 86.53% and project coverage change: +0.64 🎉

Comparison is base (32149be) 83.81% compared to head (56c28ad) 84.45%.

❗ Current head 56c28ad differs from pull request most recent head d997e6c. Consider uploading reports for the commit d997e6c to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2091      +/-   ##
==========================================
+ Coverage   83.81%   84.45%   +0.64%     
==========================================
  Files         161      143      -18     
  Lines       32395    29147    -3248     
==========================================
- Hits        27151    24617    -2534     
+ Misses       5244     4530     -714     
Impacted Files Coverage Δ
src/lib/crypto/backend_version.cpp 100.00% <ø> (+26.08%) ⬆️
src/librekey/g23_sexp.hpp 100.00% <ø> (ø)
src/librepgp/stream-packet.h 100.00% <ø> (ø)
src/tests/support.h 100.00% <ø> (ø)
src/tests/utils-rnpcfg.cpp 100.00% <ø> (ø)
src/librepgp/stream-packet.cpp 80.93% <50.00%> (-0.53%) ⬇️
src/tests/generatekey.cpp 89.73% <53.84%> (+1.05%) ⬆️
src/lib/rnp.cpp 79.88% <60.00%> (-0.18%) ⬇️
src/rnpkeys/tui.cpp 88.51% <85.71%> (+1.15%) ⬆️
src/lib/crypto/hash_common.cpp 98.11% <100.00%> (-0.14%) ⬇️
... and 8 more

... and 67 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@antonsviridenko antonsviridenko force-pushed the antonsviridenko-1609-remove-cfg-userinputfd branch from ac15f5c to 25447cf Compare June 15, 2023 19:21
@antonsviridenko
Copy link
Contributor Author

 258/259 Test #248: rnp_tests.test_fuzz_verify ....................................................   Passed    5.99 sec
terminate called after throwing an instance of 'std::bad_alloc'
  what():  std::bad_alloc
ci/lib/install_functions.inc.sh: line 676:  2438 Aborted                 (core dumped) "$@"
Error: Process completed with exit code 134.

@antonsviridenko
Copy link
Contributor Author

strange, all of the failing centos-and-fedora instances fail with the same std::bad_alloc error in a place not possibly related to the introduced changes

@ni4
Copy link
Contributor

ni4 commented Jun 19, 2023

@antonsviridenko It could be somehow related to the redirected stdin (and be thrown from GTest side), as other PRs work well.

@antonsviridenko
Copy link
Contributor Author

All failed instances use OpenSSL as a backend...

@antonsviridenko
Copy link
Contributor Author

Any ideas how to debug this?

ci/lib/install_functions.inc.sh: line 676:  2438 Aborted                 (core dumped) "$@"

Looks like it's something related to Python environment in Centos/Fedora.

@ni4
Copy link
Contributor

ni4 commented Jul 12, 2023

@antonsviridenko Did you try to revert you commits, related to stdio redirection, and see whether that helps?

@antonsviridenko
Copy link
Contributor Author

Did you try to revert you commits, related to stdio redirection, and see whether that helps?

I assume that yes, reverting commits will remove CI failures.

I think it could be related to duplicated STDIN not having CLOEXEC flags... And affected platforms run cli_tests through some python wrapper.

@antonsviridenko
Copy link
Contributor Author

Is there any possibility to recreate the same local environment as in failing centos-and-fedora machines?

@ni4
Copy link
Contributor

ni4 commented Aug 21, 2023

@antonsviridenko Wow, this one get lost somehow. Locally you may just use corresponding Docker container, install prerequisites, copy rnp sources there, build and run tests. Another option is https://github.com/nektos/act but I didn't try it yet and don't know in which way it works and is it possible to properly debug things.

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.

Check whether we may get rid of CFG_USERINPUTFD in rnpcfg. Remove CFG_USERINPUTFD
2 participants