Skip to content

Make report an allow list#1070

Merged
Marsup merged 1 commit intohapijs:masterfrom
kanongil:ts-types-fix
Jan 15, 2024
Merged

Make report an allow list#1070
Marsup merged 1 commit intohapijs:masterfrom
kanongil:ts-types-fix

Conversation

@kanongil
Copy link
Copy Markdown
Contributor

@kanongil kanongil commented Dec 28, 2023

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
@kanongil kanongil mentioned this pull request Dec 28, 2023
Merged
devinivy
devinivy approved these changes Jan 3, 2024
View reviewed changes
Copy link
Copy Markdown
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 👍

Nargonath
Nargonath approved these changes Jan 4, 2024
View reviewed changes
Copy link
Copy Markdown
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Nargonath Nargonath Nargonath approved these changes

@devinivy devinivy devinivy approved these changes

Assignees

@Marsup Marsup

Labels

bug Bug or defect

Projects

None yet

Milestone

25.1.4

Development

Successfully merging this pull request may close these issues.

4 participants

@kanongil @Nargonath @devinivy @Marsup