Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix mimetype parser wrong operator #3924

Merged
merged 1 commit into from
Dec 5, 2024

Conversation

tsctx
Copy link
Member

@tsctx tsctx commented Dec 4, 2024

The current code does not include past the end of input, so add equal sign.

@mcollina mcollina requested a review from KhafraDev December 4, 2024 13:44
@mcollina
Copy link
Member

mcollina commented Dec 4, 2024

Can you include a test? Or maybe there is a failing WPT this is fixing?

@KhafraDev
Copy link
Member

Tests passing on main: 1031
Tests passing here: 1031

@tsctx
Copy link
Member Author

tsctx commented Dec 4, 2024

This step is there to skip the rest if it is not needed, so all tests pass without this change.

position is never greater than the length of input, so it is a dead code.

For some reason, this is the only place where the greater-than was used, while other places used the correct greater-than or equal.

@KhafraDev
Copy link
Member

KhafraDev commented Dec 4, 2024

I'm not arguing with your logic (and I agree with it), but if no test failures change I would assume this code is still dead code. I would like to see a test for both of these conditions, though.

@tsctx
Copy link
Member Author

tsctx commented Dec 4, 2024

I'm not arguing with your logic (and I agree with it), but if no test failures change I would assume this code is still dead code. Should these be replaced with assertions instead?

As I mentioned before, this part of the process is to skip an extra step (at this point, an invalid entry or exit), so it is virtually untestable since it does not make any difference whether this part is there or not.

@KhafraDev
Copy link
Member

Both conditions can be caught with tests?

  1. Find an input where parseMIMEType returns failure on that condition.
  2. Find an input with an invalid parameter that triggers the if and make sure it isn't set on the returned object's parameters.

@tsctx
Copy link
Member Author

tsctx commented Dec 5, 2024

Both conditions can be caught with tests?

  1. Find an input where parseMIMEType returns failure on that condition.

  1. Find an input with an invalid parameter that triggers the if and make sure it isn't set on the returned object's parameters.

I have checked wpt mimesniff tests, these cases are covered.

@tsctx tsctx merged commit cac18e1 into nodejs:main Dec 5, 2024
29 of 31 checks passed
@tsctx tsctx deleted the fix-mimetype-parser-wrong-operator branch December 5, 2024 23:15
@KhafraDev
Copy link
Member

It would be nice to get codecov working again, so we could really see what is and isn't being tested without doing it manually.

This was referenced Dec 16, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants