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

Inconsistent error messages in test suite #1846

Closed
mohanson opened this issue Nov 13, 2024 · 4 comments
Closed

Inconsistent error messages in test suite #1846

mohanson opened this issue Nov 13, 2024 · 4 comments

Comments

@mohanson
Copy link
Contributor

mohanson commented Nov 13, 2024

I'm the author of pywasm. I'm trying to introduce more friendly error messages in my project, but I found some inconsistencies.

From spec/interpreter, the "uninitialized element" error should include the index value, but I found that the index value is both present and absent in spec/test.

| NullRef _ -> Trap.error at ("uninitialized element " ^ Int32.to_string i)

"uninitialized element 2")

(assert_trap (invoke "call" (i32.const 2)) "uninitialized element")

If the index value is forgotten in spec/test, I can submit a PR to fix spec/test.

@keithw
Copy link
Member

keithw commented Nov 13, 2024

Please see #1841 (#1076 for the history).

@mohanson
Copy link
Contributor Author

Interesting history. I submitted a PR to make the behavior consistent. #1847

@keithw
Copy link
Member

keithw commented Nov 13, 2024

I think some of the reasonable options for your test runner include:

  • only require that the test's error message be a prefix of your engine's error message (this is all that the spec interpreter does)
  • ignore the error message text entirely (which I believe several engines' testsuite runners do -- they are not normative)

I don't think you'll be happy trying to match the error text exactly -- even the spec interpreter test runner doesn't do that. But if you really want to, you could submit a #1076/#1089-style PR that goes and removes the indices everywhere they have leaked back in to the spec tests. My guess is that you'll be lonely trying to maintain that property; whatever forces motivated #1076/#1089 at the time don't seem to exist anymore, or somebody else would have complained earlier when an index was reintroduced.

@mohanson
Copy link
Contributor Author

I understand your concerns, and after evaluation I think these changes are not necessary, so I will close it, thank you.

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

No branches or pull requests

2 participants