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

[docs] Doc about testing infra #215

Merged
merged 3 commits into from
Feb 15, 2024
Merged

[docs] Doc about testing infra #215

merged 3 commits into from
Feb 15, 2024

Conversation

MCJOHN974
Copy link

No description provided.

@MCJOHN974 MCJOHN974 mentioned this pull request Feb 13, 2024

# How to use testing infra

## How test file looks like?
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure it is worth to reiterate what is effectively the contents of cranelift/filetests/README.md. Just link to that instead.

I would also perhaps consider contributing improvements to that and related files directly upstream (so long as they don't have much specific to zkasm.) I'm sure they'd appreciate more thorough docs.

Copy link
Author

Choose a reason for hiding this comment

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

Hmm, don't think I wrote something better that cranelift docs have, but I replaced it by link to docs, thanks


# CI policy

In CI we have two types of tests. One of them are written\generated by us, or adopted from existing tests and we expect that each PR passes 100% of this tests. Also, we run all tests in `filetests/filtests/runtests` as well, but don't expect all of them pass. If some test have `pass` status in main branch, and other status in the incoming branch, our CI will post a message with this diff in PR discussion, and author with reviewers will decide merge this PR or not. During project development, tests will gradually migrate from second category to the first one.

Choose a reason for hiding this comment

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

Can we instead disable/not enable tests that are not yet passing on zkasm and keep all enabled tests always green? This way we don't need any new CI machinery and conventions during PR review.

Copy link
Author

Choose a reason for hiding this comment

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

I was thinking it will be nice to not disable test, but make it not mandatory and still know it's status. But maybe it is really not worth creating any new CI machinery

Copy link
Author

Choose a reason for hiding this comment

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

New commit removes this part

Copy link

@aborg-dev aborg-dev left a comment

Choose a reason for hiding this comment

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

LGTM

@MCJOHN974 MCJOHN974 added this pull request to the merge queue Feb 15, 2024
Merged via the queue into main with commit 8032feb Feb 15, 2024
21 checks passed
@MCJOHN974 MCJOHN974 deleted the viktar/test-docs branch February 15, 2024 11:14
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.

3 participants