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

Integration tests avoid tmp dir #1359

Merged

Conversation

plebhash
Copy link
Collaborator

@plebhash plebhash commented Jan 18, 2025

closes #1278, alternative to #1342

unfortunately #1293 didn't fix #1278

this PR changes the directory where TP is downloaded into, replacing /tmp (which was giving permission errors in an undeterministic way) to a new template-provider directory crated at the Integration Test execution (which shouldn't ever have permission issues)

this new directory is listed on a new .gitignore so that we don't commit binary executables to the SRI repo, which would be bad practice

Copy link

codecov bot commented Jan 18, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 19.09%. Comparing base (7be7d5e) to head (f9354c3).
Report is 3 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1359   +/-   ##
=======================================
  Coverage   19.09%   19.09%           
=======================================
  Files         166      166           
  Lines       11062    11062           
=======================================
  Hits         2112     2112           
  Misses       8950     8950           
Flag Coverage Δ
binary_codec_sv2-coverage 0.00% <ø> (ø)
binary_serde_sv2-coverage 3.55% <ø> (ø)
binary_sv2-coverage 5.34% <ø> (ø)
bip32_derivation-coverage 0.00% <ø> (ø)
buffer_sv2-coverage 25.02% <ø> (ø)
codec_sv2-coverage 0.01% <ø> (ø)
common_messages_sv2-coverage 0.13% <ø> (ø)
const_sv2-coverage 0.00% <ø> (ø)
error_handling-coverage 0.00% <ø> (ø)
framing_sv2-coverage 0.28% <ø> (ø)
jd_client-coverage 0.00% <ø> (ø)
jd_server-coverage 7.79% <ø> (ø)
job_declaration_sv2-coverage 0.00% <ø> (ø)
key-utils-coverage 2.39% <ø> (ø)
mining-coverage 2.44% <ø> (+0.01%) ⬆️
mining_device-coverage 0.00% <ø> (ø)
mining_proxy_sv2-coverage 0.70% <ø> (ø)
noise_sv2-coverage 4.44% <ø> (ø)
pool_sv2-coverage 2.05% <ø> (ø)
protocols 24.57% <ø> (ø)
roles 6.55% <ø> (ø)
roles_logic_sv2-coverage 7.93% <ø> (ø)
sv2_ffi-coverage 0.00% <ø> (ø)
template_distribution_sv2-coverage 0.00% <ø> (ø)
translator_sv2-coverage 9.60% <ø> (ø)
utils 25.13% <ø> (ø)
v1-coverage 2.41% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

Copy link
Contributor

@Shourya742 Shourya742 left a comment

Choose a reason for hiding this comment

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

ACK on the current changes. However, I am not entirely sure about the implementation of removing the template provider directory in the Drop trait. If this is done, all the tests might take significantly longer since they would need to download the binary every time or might fail due to download errors. #1342 (review).

@plebhash
Copy link
Collaborator Author

ACK on the current changes. However, I am not entirely sure about the implementation of removing the template provider directory in the Drop trait. If this is done, all the tests might take significantly longer since they would need to download the binary every time or might fail due to download errors. #1342 (review).

can you elaborate? where are we removing the TP directory? there's no Drop trait implementation for the template_provider module, and the Drop trait implementation from underlying bitcoind crate simply stops the process.

@Shourya742
Copy link
Contributor

ACK on the current changes. However, I am not entirely sure about the implementation of removing the template provider directory in the Drop trait. If this is done, all the tests might take significantly longer since they would need to download the binary every time or might fail due to download errors. #1342 (review).

can you elaborate? where are we removing the TP directory? there's no Drop trait implementation for the template_provider module, and the Drop trait implementation from underlying bitcoind crate simply stops the process.

I was mentioning about Esraa's suggestion...

@plebhash
Copy link
Collaborator Author

plebhash commented Jan 20, 2025

ACK on the current changes. However, I am not entirely sure about the implementation of removing the template provider directory in the Drop trait. If this is done, all the tests might take significantly longer since they would need to download the binary every time or might fail due to download errors. #1342 (review).

can you elaborate? where are we removing the TP directory? there's no Drop trait implementation for the template_provider module, and the Drop trait implementation from underlying bitcoind crate simply stops the process.

I was mentioning about Esraa's suggestion...

ah ok sorry I missed the link on the end of the comment

yeah I didn't go in that direction... instead, I marked the new directory on .gitignore

Copy link
Contributor

@jbesraa jbesraa left a comment

Choose a reason for hiding this comment

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

LGTM. Do you think this is something worth mentioning in the readme? we are writing to a user disk and we might end up taking a good chunk of space.

roles/tests-integration/.gitignore Outdated Show resolved Hide resolved
@plebhash plebhash force-pushed the integration-tests-avoid-tmp-dir branch 2 times, most recently from 570ee84 to ed8d2a2 Compare January 21, 2025 14:32
@plebhash plebhash requested a review from jbesraa January 21, 2025 14:32
@plebhash plebhash added the ready-to-be-merged triggers auto rebase bot label Jan 21, 2025
@pavlenex pavlenex force-pushed the integration-tests-avoid-tmp-dir branch from ed8d2a2 to 2e92611 Compare January 21, 2025 16:59
@pavlenex pavlenex force-pushed the integration-tests-avoid-tmp-dir branch from 2e92611 to 7030a61 Compare January 21, 2025 18:20
using /tmp is leading to undeterministic behavior on Github CI
to avoid committing the `template-provider` directory created during
execution of Integration Tests
@pavlenex pavlenex force-pushed the integration-tests-avoid-tmp-dir branch from 7030a61 to f9354c3 Compare January 21, 2025 21:33
@plebhash plebhash merged commit 7a39869 into stratum-mining:main Jan 21, 2025
33 checks passed
@plebhash plebhash deleted the integration-tests-avoid-tmp-dir branch January 21, 2025 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test TemplateProvider OS E13
4 participants