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

Add a check if substitutions exists #46945

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

asamuzaK
Copy link

Fix #46940

@Ms2ger
Copy link
Contributor

Ms2ger commented Jul 1, 2024

Why is the argument undefined, though?

@asamuzaK
Copy link
Author

asamuzaK commented Jul 1, 2024

I do not know why.
It may be a jsdom specific issue.

Here's a simple way to reproduce the issue.

  • Save a clone of jsdom to your local PC.
  • Copy the latest source of https://raw.githubusercontent.com/web-platform-tests/wpt/master/resources/testharness.js and paste it into local jsdom/test/web-platform-tests/tests/resources/testharness.js.
  • Run npm test.

@asamuzaK
Copy link
Author

asamuzaK commented Jul 2, 2024

I took a brief look at the source.

In L4514:

    function render(template, substitutions, output_document)

render() accepts 3 arguments, and the 2nd is the substitutions.

However, for example, in L4290, only 1 argument is specified.
So substitutions will be undefined.

            const asserts_output = render(
                ["details", {},
                    ["summary", {}, "Asserts run"],
                    ["table", {}, ""] ]);

By the way, the previous PR had an array as the default value, which was a mistake.
substitutions[components[i + 1]] is a property of substitutions.

substitutions = {
  [components[i + 1]]: foo
}

So, fixed default value to substitutions = {}.

@asamuzaK asamuzaK marked this pull request as draft July 3, 2024 01:55
@asamuzaK asamuzaK marked this pull request as draft July 3, 2024 01:55
@asamuzaK asamuzaK marked this pull request as draft July 3, 2024 01:55
@asamuzaK asamuzaK changed the title Add default value to substitute_single function Add a check if substitutions exists Jul 3, 2024
@asamuzaK asamuzaK marked this pull request as ready for review July 3, 2024 04:29
@asamuzaK
Copy link
Author

asamuzaK commented Jul 3, 2024

Ready for review now.
Please take a look.

Some notes about the issue:

function substitute_single(template, substitutions) accepts two arguments, but the second subustitutions may be undefined.
See #issuecomment-2203051749 above.

On the other hand, in the internal function function do_substitution(input), it is assuming that substitutions always exists.
Or if substitutions is undefined, the length of components would always be 1.

Therefore, if substitutions is undefined and components.length is greater than 1, exception raises at substitutions[components[i + 1]];.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

testharness.js throws in some cases
4 participants