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

Commit small test json files for each test instead of the large ones #509

Merged
merged 15 commits into from
Feb 13, 2025

Conversation

alan-ssvlabs
Copy link
Contributor

@alan-ssvlabs alan-ssvlabs commented Oct 8, 2024

Fixes #482

This PR creates one json file for each test. Large tests.json files are still generated but no longer committed.

The tests still use large tests.json files to run, the small json files are to keep track of changes easily.

Copy link
Contributor

@MatheusFranco99 MatheusFranco99 left a comment

Choose a reason for hiding this comment

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

Well done!

The tests.json files were removed in the ssv and types modules, but not in the qbft module. Is there a reason for that? It seems to me that all should be removed

Copy link
Contributor

@GalRogozinski GalRogozinski left a comment

Choose a reason for hiding this comment

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

Good job!

A few requests besides my comment:

  1. Can you pretty print the jsons into files. This will help with tracking changes without external tools
  2. I think it will be a good idea to add arg to mains so that by default will generate the big file only (to not change CI process). But with a given option will only generate small files. This will make us run faster.

Good job overall!

@GalRogozinski
Copy link
Contributor

@alan-ssvlabs
Also change description so it will automatically close the issue (use "close" or "fix")
See: https://docs.github.com/en/issues/tracking-your-work-with-issues/using-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword

@alan-ssvlabs
Copy link
Contributor Author

alan-ssvlabs commented Nov 20, 2024

Thanks @GalRogozinski, updated the description and made the suggested changes

But for the file permissions it fails the gosec because gosec suggests 600 or less, and it raises permission denied for the json files when using git add .

@GalRogozinski
Copy link
Contributor

@alan-ssvlabs
So maybe do 644?
Basically I want everyone to be able to read... If owner can write I guess it is ok

@alan-ssvlabs
Copy link
Contributor Author

@alan-ssvlabs So maybe do 644? Basically I want everyone to be able to read... If owner can write I guess it is ok

Did it as 644, gosec is not happy because it expect 600 or less so I added nosec. Also the permission denied messages are still there on git add...

@GalRogozinski
Copy link
Contributor

@alan-ssvlabs
not sure why with 0644 you are getting those

@alan-ssvlabs
Copy link
Contributor Author

Very strangely I didn't get any permission errors before editing this 444 or 644 permissions, now no matter how I set it I get permission errors for both go generate and git add. I suspect it is my local problem, can you clone this branch and try to generate and git add? @GalRogozinski

@GalRogozinski
Copy link
Contributor

@alan-ssvlabs
I am currently getting:

types/spectest/generate/tests/committeemember.CommitteeMemberTest_committee_member_has_quorum_3f1.json: Permission denied
types/spectest/generate/tests/committeemember.CommitteeMemberTest_committee_member_no_quorum_duplicate.json: Permission denied
types/spectest/generate/tests/committeemember.CommitteeMemberTest_committee_member_quorum_with_duplicate.json: Permission denied
types/spectest/generate/tests/consensusdataproposer.ProposerSpecTest_consensus_data_versioned_blinded_block_corrupted_consensus_data.json: Permission denied
types/spectest/generate/tests/consensusdataproposer.ProposerSpecTest_consensus_data_versioned_blinded_block_unknown_version.json: Permission denied
types/spectest/generate/tests/consensusdataproposer.ProposerSpecTest_consensus_data_versioned_blinded_block_validation.json: Permission denied

I think what is happening is once the files were created once w.o write permission it is hard for them to be deleted.
So locally do a chmod on all of them
and only use 0x644

@GalRogozinski
Copy link
Contributor

Seems I gotten myself to a position where I get permission errors always unless if I use the SUDO flag
and no sudo chmod helps even if permission changes
Had to delete everything to make it work...

maybe we go back to original permissions if this causes issues.
It isn't the biggest deal... It was supposed to be easy

Copy link
Contributor

@GalRogozinski GalRogozinski left a comment

Choose a reason for hiding this comment

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

we should pretty print the jsons

Copy link
Contributor

@y0sher y0sher left a comment

Choose a reason for hiding this comment

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

lgtm, can you leave some description for how to generate the files if needed for testing? now that they are not in the repo.

@GalRogozinski
Copy link
Contributor

@y0sher
they are generated as normal just no longer committed
btw, we should start changing our CI to actually use only the small files imo
I didn't want to hinder the release for now, so this is how it is done now

@GalRogozinski GalRogozinski merged commit ee6620b into ssvlabs:main Feb 13, 2025
2 checks passed
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.

Seperate tests.json files to smaller files
5 participants