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

Fix the expected result of expr1 #56

Closed
wants to merge 1 commit into from
Closed

Conversation

ndw
Copy link
Contributor

@ndw ndw commented Mar 21, 2022

This is the test that will not rest.

I believe we've agreed that the result of this test should be an error. However, the test catalog currently asserts that it is "not a grammar". I don't think that's correct. This PR changes it to "not a sentence" which is better.

The description of the test suggests that we may need a new assertion. Perhaps that would be best. We could add, for example, assert-not-xml. If anyone thinks that's substantially better, just say so.

@cmsmcq I think you're the de facto maintainer of the test schemas, but if it's more convenient, I can make the change and send you a PR for the schemas.

@ndw ndw added the testsuite An issue related to the test suite label Apr 3, 2022
@ndw ndw added this to the Version 1.0 milestone Apr 3, 2022
@cmsmcq
Copy link
Contributor

cmsmcq commented May 24, 2022

For what it's worth, I think the correct expectation for this test is assert-dynamic-error. The entry in the copy of this test catalog I am using reads:

  <test-case name="expr1">
    <created by="SP" on="2021-12-16"/>
    <modified by="MSM" on="2021-12-23"
              change="Change result"/>
    <modified by="MSM" on="2022-01-01"
              change="remove trailing whitespace from input"/>
    <modified by="MSM" on="2022-01-01"
              change="remove whitespace from expected result"/>
    <modified by="MSM" on="2022-05-21"
              change="change to assert-dynamic-error"/>
    <test-string-ref href="expr1.inp"/>
    <description>
      <p>Illustrates the way to handle dynamic errors in a test:
      use assert-dynamic-error.</p>
      <p>It's not formally a grammar error, since this grammar
      will work fine for some inputs and processors are not
      required to reject the grammar.  So assert-not-a-grammar
      is not the right thing.</p>
      <p>It's not formally a failure to parse the input, since
      this input string is perfectly grammatical against this
      input grammar.  So assert-not-a-sentence is not the right
      thing.</p>
      <p>The error arises in serialization and only at run time.
      So assert-dynamic-error.</p>
    </description>
    <result>
      <assert-dynamic-error/>
      <!--
      <assert-xml-ref href="expr1.output.xml"/>
      -->
    </result>
  </test-case>

In due course (I hope in the course of today) I will formulate a pull request to make this change.

@ndw
Copy link
Contributor Author

ndw commented May 24, 2022

That works for me. Thank you, Michael.

@yamahito
Copy link
Contributor

Agree with Michael

@ndw
Copy link
Contributor Author

ndw commented May 31, 2022

Resolved by a recent merge, as described above.

@ndw ndw closed this May 31, 2022
@ndw ndw deleted the fix-expr1 branch October 15, 2022 05:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testsuite An issue related to the test suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants