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

Test harness: parse and ignore UPSTREAM tag #2463

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

SoniEx2
Copy link
Contributor

@SoniEx2 SoniEx2 commented Sep 12, 2024

This is a funny idea we have, but it'd help a lot with maintenance if we could tag certain tests with an UPSTREAM: [commit ID] tag and then automatically check those tags against the testsuite. An initial step would be to parse such tag, but simply ignore it for now.

The idea being, wabt doesn't track the latest version of the testsuite all the time, yet wabt regularly upstreams regressions to the testsuite. For deduplication reasons, it'd be nice if we could automatically check the testsuite version and see if the local copy of the tests is no longer needed.

Thoughts?

@SoniEx2
Copy link
Contributor Author

SoniEx2 commented Sep 12, 2024

(huh, spurious CI failure?)

Copy link
Member

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

Sounds reasonable.

We could also just use the existing TODO or NOTE for this purpose.

@SoniEx2
Copy link
Contributor Author

SoniEx2 commented Sep 12, 2024

we don't like the idea of embedding machine-readable metadata into the NOTE/TODO tags... if we're gonna be developing the tools for this, may as well use the tag system.

@sbc100
Copy link
Member

sbc100 commented Sep 12, 2024

we don't like the idea of embedding machine-readable metadata into the NOTE/TODO tags... if we're gonna be developing the tools for this, may as well use the tag system.

I'm not sure this is going to be common enough to make it worthwhile to add a lot of automation. A simple git grep NOTE test should be enough to see all the places the we mention upstream revisions. I imagine there will only be 1 or 2 of these at a given time.

@SoniEx2
Copy link
Contributor Author

SoniEx2 commented Sep 12, 2024

oh. in which case, a separate tag would still be convenient, as it could be checked when updating the testsuite without having to sift through existing uses of TODO/NOTE.

@SoniEx2
Copy link
Contributor Author

SoniEx2 commented Sep 13, 2024

for context:

$ grep -r -a ';; NOTE' test/
test/regress/regress-3.txt:;;; NOTE: bug when calling function defined with type after call
test/regress/regress-25.txt:;;; NOTE: infinite loop when \0 character is in text
test/decompile/names.txt:;; NOTE: same test as in test/binary/names.txt
test/dump/elem-mvp-compat-named.txt:;;; NOTE: referring to table 0 by name should still be MVP compatible
test/dump/elem-mvp-compat.txt:;;; NOTE: using (table 0) syntax should still be MVP compatible when possible
test/binary/names.txt:;; NOTE: same test as in test/decompile/names.txt
$ grep -r -a ';; TODO' test/
test/interp/convert.txt:  ;; TODO(binji): how best to distinguish _s from _u?
test/dump/invalid-init-exprs.txt:;; TODO: wasm-objdump should handle the "<INVALID>" case more gracefully

(hmm, looks like the TODO/NOTE tags aren't even used consistently...)

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.

2 participants