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

Set Warning Header when handling preproc extension and validation #155

Merged
merged 1 commit into from
Aug 3, 2016

Conversation

RackerWilliams
Copy link
Contributor

See RFC 2616 (14.46)

The preproc extension transforms a message, we should be setting an appropriate Warning header.

@RackerWilliams RackerWilliams modified the milestones: 2.0.1, Backlog Jun 9, 2016
@RackerWilliams
Copy link
Contributor Author

...a better description on RFC 7234.

@wdschei
Copy link
Contributor

wdschei commented Jul 29, 2016

Links to the referenced docs:

@wdschei
Copy link
Contributor

wdschei commented Jul 29, 2016

We probably need to think about breaking the ValidatorWADLSuite of tests up since it is pushing almost 3k lines.

@@ -146,7 +146,7 @@ class WADLCheckerHeaderSingleSpec extends BaseCheckerSpec with LogAssertions {
XML.loadFile("src/test/resources/xsd/test-urlxsd.xsd"))
When("the wadl is translated")
val checker = builder.build (inWADL, TestConfig(false, false, true, true, true, 1,
true, true, true, "XalanC",
true, false, true, "XalanC",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why were all of these changed to no longer do the XSD Grammar Transform?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These "checker" tests -- essentially confirm that the sate machine is created as expected. None of these tests test functionality related to the Grammar Transform, that's not the purpose of these tests, but in the past checking grammar didn't really affect the state machine in a way that the assertions in these tests check. Now that warnings are enabled by default the state machines changed so to prevent further changes, I've simply disabled setting the grammar transform in the config here. Actual grammar transform tests are checked elsewhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense.

@wdschei
Copy link
Contributor

wdschei commented Jul 29, 2016

Wow. I just did a line count to see how many of those huge files there are and these are the ones over 2k lines each:

  • 2573 ./core/src/test/scala/com/rackspace/com/papi/components/checker/ValidatorWADLSuite.scala
  • 3186 ./core/src/test/scala/com/rackspace/com/papi/components/checker/ValidatorWADLHeaderSuite.scala
  • 3484 ./core/src/test/scala/com/rackspace/com/papi/components/checker/ValidatorWADLHeaderSuite2.scala
  • 3849 ./core/src/test/scala/com/rackspace/com/papi/components/checker/wadl/WADLCheckerAssertSpec.scala
  • 6274 ./core/src/test/scala/com/rackspace/com/papi/components/checker/wadl/WADLCheckerHeaderSingleSpec.scala
  • 6509 ./core/src/test/scala/com/rackspace/com/papi/components/checker/step/StepSuite.scala
  • 15667 ./core/src/test/scala/com/rackspace/com/papi/components/checker/wadl/WADLCheckerSpec.scala

reqTypeAssertions(checker)
wellFormedAssertions(checker)
xsdAssertions(checker)
And("The following assertions should also hold:")
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be Then("the...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I'm really starting to regret that pattern here. Since this doesn't change the assertions that are checked I'll save these type of changes for #109.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I guess this is a new test so I'll go ahead and change it.

@wdschei
Copy link
Contributor

wdschei commented Jul 29, 2016

This looks good to me. I just have the one question about no longer doing the XSD Grammar Transform and the couple of minor formatting things.

@RackerWilliams
Copy link
Contributor Author

re: The length of some of the test files. I couldn't agree more I have an open issue on this #109.

See RFC 7234 for description of warning header.
We add a new step "SetHeaderAlways" to implement this.
@RackerWilliams
Copy link
Contributor Author

Added formatting fixes.

@wdschei
Copy link
Contributor

wdschei commented Aug 3, 2016

Glad to hear about #109.
Looks good to me.

@wdschei wdschei merged commit 71ace27 into rackerlabs:master Aug 3, 2016
@RackerWilliams RackerWilliams deleted the transform_warning branch August 6, 2016 05:31
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.

2 participants