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

Parse all tests #36

Merged
merged 2 commits into from
Aug 21, 2023
Merged

Conversation

Nashtare
Copy link
Contributor

This PR adds an extra flattening step pre test parsing to accommodate for the particular structure of the Shanghai and VMTests folders.

@BGluth Not sure my approach is super clean, hence welcoming feedback more than ever! 😅

closes #33

@Nashtare Nashtare requested a review from BGluth August 19, 2023 18:32
@Nashtare Nashtare self-assigned this Aug 19, 2023
@Nashtare
Copy link
Contributor Author

Note that clippy recommends using PermissionsExt, as set_readonly(false) leads to a file that is wold writeable, though I haven't been able to fix the writing rights with PermissionsExt, I suspect it may be coming from the user's umask messing up permissions?

Copy link
Contributor

@BGluth BGluth left a comment

Choose a reason for hiding this comment

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

Yeah I think your approach is good!

My unfinished attempt involved adjusting the logic in fs_scaffolding.rs to drill down into sub-directories until we found directories with just .json files, but your approach is actually looking a bit cleaner than mine.

I'm not getting the clippy warnings you were getting for whatever reason though, so I'm not sure what's up with that. It seems to run fine if I get rid of the write permission logic that you added, but if it was causing an issue on your end I'm fine to leave it in (as it might cause issues for someone else).

@Nashtare
Copy link
Contributor Author

Hmm yeah I couldn't copy the files in the subfolders without changing the writing permissions, not sure what's the underlying cause if you're not getting this on your end..

@BGluth
Copy link
Contributor

BGluth commented Aug 21, 2023

Regardless I'm good to merge. Don't feel like you need to dig too deep into the cause if it's going to be a time sink on your end. 👍

@Nashtare Nashtare merged commit ae799c2 into 0xPolygonZero:main Aug 21, 2023
2 checks passed
@Nashtare Nashtare deleted the parse_all_tests branch August 30, 2023 14:35
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.

Test parser is missing some tests
2 participants