Improving WARequestTest >> testPostFields

[mabdi]

Hi,

I submit this pull request to suggest some improvements in the test method #testPostFields to cover some corner cases in the class WARequest.

By including the lines #5-#10 the test will now execute methods in the class under test which previously were not covered by WARequestTest (isXmlHttpRequest, headers, remoteAddress, isPost, sslSessionId). This will also guard against a WARRequest object which is improperly initialized. Line #6 verifies that the WARRequest does not have an XMLHttpRequest header by default. Line #9 on the other hand verifies that WARequest is properly initialized via #method:uri: and ‘POST’ argument.

By including Line #15, the test will guard against sending #bodyDecoded when charSet isNil (a WAMimeType(application/x-www-form-urlencoded) has a nil charSet).

Note that these suggestions are adapted from a test amplification tool called SmallAmp (https://github.com/mabdi/small-amp). SmallAmp executes existing tests, sees which parts of the class under test are not covered and then suggests improvements on the test methods.

I hope you will accept this pull request. It would illustrate that SmallAmp makes relevant suggestions.

Mehrdad Abdi.

[sergedemeyer]

This pull request has previously been submitted on the development branch. There all builds failed on Jenkins, because JSScriptTest >> #testAliasAndFunctionCompositions does fail. This wasn't affected by our pull request, hence it is likely from a previous failing build.

The pull request was closed on the development branch and reopened on the main branch. There it fared better: it passes all tests on Pharo but remains problematic on Squeak and Gemstone. Again, the failures wre not caused by this pull request but had an outside cause.

[jbrichau]

@sergedemeyer Hi Serge! Yes, the build infrastructure has had a couple of bad days. I restarted the build for this PR a couple of days back to get that cleared up. The Squeak build has a failing test for now, which we now should be fixed soon too and is not related to this PR.

@mabdi Thank you for sending in this test enhancement. We certainly welcome test enhancements. Given that I think this is part of research application, I will take the freedom to give some more feedback ;)

At first glance, a quibble I had with the given unit test extension is that I would expect these additional test assertions to finally end up in other unit tests. I expected the testPostfields unit test method to focuse on testing the postFields method of WARequest. However, I now notice it also sends setBody:, which is why I suspect the test amplification tool suggested to add a test for bodyDecoded. As a human, this makes me want to move that suggested test amplication to a new unit test method testBody (or testBodyDecoded) and write up the entire test case for that method more as follows:

...
self should: [ request bodyDecoded ] raise: WAIllegalStateException.
request setPostFields: (WARequestFields new at: 'baz' put: '2'; at: 'bar' put: '3'; yourself).      request setPostFields: (WARequestFields new at: 'baz' put: '2'; at: 'bar' put: '3'; yourself).
self shouldnt: [ decodedBody :=request bodyDecoded ] raise: WAIllegalStateException.
self assert: decodedBody equals: ...

So, in summary: I will merge in these enhancements but, more importantly, the result of the test amplification makes me evaluate the existing unit tests and refactor them to improve the test coverage and test factorization.

It would be interesting to see more of what the tool suggests (although my time right now is too limited and full of other engagements, so probably something for after summer).