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

Dont honour params specified in suite-file tag #2923

Merged
merged 1 commit into from
Jun 8, 2023

Conversation

krmahadevan
Copy link
Member

Closes #581

TestNG does not have any logic that would
honour parameter tags specified within suite-files tag. But due to a parsing bug, we end up reading
parameters that were specified within
<suite-file> tag.
This tag by definition is NOT meant to have any
child tags inside of it.

Fixed this discrepancy by adding a warning when
this anomaly is detected and skipping of reading
the <parameter> tag inside it as if it were
specified within <suite> tag.

Fixes #581 .

Did you remember to?

  • Add test case(s)
  • Update CHANGES.txt
  • Auto applied styling via ./gradlew autostyleApply

We encourage pull requests that:

  • Add new features to TestNG (or)
  • Fix bugs in TestNG

If your pull request involves fixing SonarQube issues then we would suggest that you please discuss this with the
TestNG-dev before you spend time working on it.

Note: For more information on contribution guidelines please make sure you refer our Contributing section for detailed set of steps.

Copy link
Member

@juherr juherr left a comment

Choose a reason for hiding this comment

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

The dtd file should be updated to be synch to the behavior

@krmahadevan
Copy link
Member Author

The dtd file should be updated to be synch to the behavior

I would need help with that, because I have never done that. Also how do we go about publishing this ?

@juherr
Copy link
Member

juherr commented Jun 7, 2023

I checked and confirm that the issue is in the dtd file:


<!ELEMENT suite-file ANY >
<!ATTLIST suite-file
    path CDATA #REQUIRED
>

According to the spec, suite-file should be EMPTY and not ANY.
I think we should check every element node.

The dtd files are hosted here: https://github.com/testng-team/testng-team.github.io

@krmahadevan
Copy link
Member Author

@juherr - The DTD is fixed as part of this PR testng-team/testng-team.github.io#46

Can we go ahead with merging this PR ?

@krmahadevan krmahadevan force-pushed the bugfix/581 branch 2 times, most recently from 0ac19c0 to c8bfea8 Compare June 7, 2023 13:16
@krmahadevan krmahadevan closed this Jun 7, 2023
@krmahadevan krmahadevan deleted the bugfix/581 branch June 7, 2023 13:21
@krmahadevan krmahadevan restored the bugfix/581 branch June 7, 2023 13:21
@krmahadevan krmahadevan reopened this Jun 7, 2023
Closes testng-team#581

TestNG does not have any logic that would
honour parameter tags specified within suite-files
tag. But due to a parsing bug, we end up reading
parameters that were specified within
`<suite-file>` tag.
This tag by definition is NOT meant to have any
child tags inside of it.

Fixed this discrepancy by adding a warning when
this anomaly is detected and skipping of reading
the `<parameter>` tag inside it as if it were
specified within `<suite>` tag.
Copy link
Member

@juherr juherr left a comment

Choose a reason for hiding this comment

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

LGTM. Just be sure that the new dtd version works well together with the previous one

@krmahadevan krmahadevan merged commit 1288058 into testng-team:master Jun 8, 2023
@krmahadevan krmahadevan deleted the bugfix/581 branch June 8, 2023 03:08
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

Successfully merging this pull request may close these issues.

Parameters of nested test suites are overriden
2 participants