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

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

Conversation

alan-ssvlabs
Copy link

@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

qbft/spectest/generate/main.go Show resolved Hide resolved
types/spectest/generate/main.go Outdated Show resolved Hide resolved
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!

types/spectest/generate/main.go Outdated Show resolved Hide resolved
types/spectest/generate/main.go Outdated Show resolved Hide resolved
types/spectest/generate/main.go Show resolved Hide resolved
qbft/spectest/generate/main.go Outdated Show resolved Hide resolved
qbft/spectest/generate/main.go Outdated Show resolved Hide resolved
.gitignore Show resolved Hide resolved
ssv/spectest/generate/main.go Outdated Show resolved Hide resolved
ssv/spectest/generate/main.go Outdated Show resolved Hide resolved
ssv/spectest/generate/main.go Show resolved Hide resolved
ssv/spectest/generate/main.go Show resolved Hide resolved
@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
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
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
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

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
3 participants