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

Make report an allow list #1070

Merged
merged 1 commit into from
Jan 15, 2024
Merged

Make report an allow list #1070

merged 1 commit into from
Jan 15, 2024

Conversation

kanongil
Copy link
Contributor

For some reason the "report" list, which is used limit the errors that can be inside expect.error(), is a deny list. This causes problems when upgrading typescript, which can report new error codes for an existing test case. Since the new error code is not a part of the deny list, it won't be handled by expect.error() checks, causing the expect to fail, even though it still errors.

This can be seen in @hapi/hoek, where updating typescript to v5.3.3, will cause the types check to fail:

	test/index.ts:35:38: Object literal may only specify known properties, and 'unknown' does not exist in type 'Options'.
	test/index.ts:58:30: Object literal may only specify known properties, and 'unknown' does not exist in type 'Options'.
	test/index.ts:74:49: Object literal may only specify known properties, and 'unknown' does not exist in type 'Options'.
	test/index.ts:183:40: Object literal may only specify known properties, and 'unknown' does not exist in type 'Options'.
	test/index.ts:201:50: Object literal may only specify known properties, and 'unknown' does not exist in type 'Options'.
	test/index.ts:35:0: Expected an error
	test/index.ts:58:0: Expected an error
	test/index.ts:74:0: Expected an error
	test/index.ts:183:0: Expected an error
	test/index.ts:201:0: Expected an error

We could add this new code to the list, but it will be a never ending a cat and mouse game. Additionally, there are likely many other codes that would make sense to be expect.error()-able.

The best solution seems to be to change it to an allow list. In this case there is only one code 1005, which is already tested for in the "identifies errors" test case.

@kanongil kanongil added the bug Bug or defect label Dec 28, 2023
Copy link
Member

@devinivy devinivy left a comment

Choose a reason for hiding this comment

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

It certainly has been a cat and mouse game! I think this is a good call 👍

Copy link
Member

@Nargonath Nargonath left a comment

Choose a reason for hiding this comment

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

LGTM, thanks Gil.

@Marsup Marsup added this to the 25.1.4 milestone Jan 15, 2024
@Marsup Marsup self-assigned this Jan 15, 2024
@Marsup Marsup merged commit 61215b6 into hapijs:master Jan 15, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug or defect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants