Skip to content

Conversation

Narigo
Copy link
Member

@Narigo Narigo commented Nov 5, 2013

Please review and merge if okay.

@@ -19,14 +19,12 @@ class HttpTestForSimpleApi extends TestVerticle {
</body>
</html>.toString()

@Test
Copy link
Member

Choose a reason for hiding this comment

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

Why remove the annotations ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Somehow it doesn't work in Scala to have the @Test methods in a superclass. The gradle test runner then always yields something like Cannot find method XXX and fails the tests. The tests now are in the subclasses and use the super implementation.

Copy link
Member

Choose a reason for hiding this comment

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

Strange... I would have expected it would "just work"

Copy link
Member Author

Choose a reason for hiding this comment

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

You can try it yourself, maybe it's just my environment and we can figure out how to make this even more simpler. But I guess there's some issue with Scala + JUnit and this should work for everyone now. :)

Copy link
Member

Choose a reason for hiding this comment

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

I think it's ok was just wondering

Am 05.11.2013 um 07:58 schrieb Joern Bernhardt notifications@github.com:

In src/test/scala/org/vertx/scala/tests/core/http/HttpTestForSimpleApi.scala:

@@ -19,14 +19,12 @@ class HttpTestForSimpleApi extends TestVerticle {

.toString()

  • @test
    You can try it yourself, maybe it's just my environment and we can figure out how to make this even more simpler. But I guess there's some issue with Scala + JUnit and this should work for everyone now. :)


Reply to this email directly or view it on GitHub.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Narigo @normanmaurer WDYT of galderz@243592c ? Runs the test function with both clients, compressed and uncompressed.

Copy link
Member Author

Choose a reason for hiding this comment

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

@galderz just skimmed over it - will we be able to see which one of the two failed? Do we get some kind of message "Now testing with/without compression" ? If not, can we at least make a logger.info() or something like that?

From the point of reusability I'd say let's use your version, definitely better to just have a single class for the tests.

Copy link
Member

Choose a reason for hiding this comment

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

Agree here

Am 14.11.2013 um 17:50 schrieb Joern Bernhardt notifications@github.com:

In src/test/scala/org/vertx/scala/tests/core/http/HttpTestForSimpleApi.scala:

@@ -19,14 +19,12 @@ class HttpTestForSimpleApi extends TestVerticle {

.toString()

  • @test
    @galderz just skimmed over it - will we be able to see which one of the two failed? Do we get some kind of message "Now testing with/without compression" ? If not, can we at least make a logger.info() or something like that?

From the point of reusability I'd say let's use your version, definitely better to just have a single class for the tests.


Reply to this email directly or view it on GitHub.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Narigo No, at the moment I don't think you'd be able to see which one failed, but I can improve on that for sure. I'll update asap.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Narigo Done. I've submitted a new PR (#95) with your changes and my test adjustments. I've added information to the assert messages on whether the client has compression enabled or not.

@galderz
Copy link
Contributor

galderz commented Nov 21, 2013

Closing this. I've added a new PR (#95) with @Narigo's code and my test adjustments.

@galderz galderz closed this Nov 21, 2013
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.

3 participants