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

Bug 62785. -Incomplete search path applied to the filenames used in the upload functionality of the HTTP sampler #400

Closed

Conversation

@artem-fedorov
Copy link

@artem-fedorov artem-fedorov commented Oct 1, 2018

https://bz.apache.org/bugzilla/show_bug.cgi?id=62785

@artem-fedorov artem-fedorov changed the title Bug 62785. - HTTP Request throws java.io.FileNotFoundException when upload file without param name and MIME type Bug 62785. -Incomplete search path applied to the filenames used in the upload functionality of the HTTP sampler Oct 2, 2018
@asfgit asfgit closed this in 86d466b Oct 3, 2018
HTTPSamplerBase sampler = new HTTPSampler3();
sampler.setMethod("POST");
sampler.setPath("http://httpbin.org/post");
sampler.setHTTPFiles(new HTTPFileArg[]{new HTTPFileArg("bin/jmeter.properties", "", "")});
Copy link
Collaborator

@vlsi vlsi Feb 27, 2019

Why, why do you people use bin/jmeter.properties for unit testing?
Eh :(

Copy link
Contributor

@FSchumacher FSchumacher Feb 28, 2019

We have introduced a resources directory under test that should be used for test files -- and those should be test specific, but it was either really old code, or someone was lazy.


SampleResult sample = sampler.sample();
System.out.println(sample.getResponseDataAsString());
assertFalse(sample.getResponseDataAsString().contains("java.io.FileNotFoundException:"));
Copy link
Collaborator

@vlsi vlsi Feb 27, 2019

@artem-fedorov , @FSchumacher , please refrain from computations inside asserttrue/false.
It produces a failure like the following

java.lang.AssertionError
        at org.junit.Assert.fail(Assert.java:86)
        at org.junit.Assert.assertTrue(Assert.java:41)
        at org.junit.Assert.assertFalse(Assert.java:64)
        at org.junit.Assert.assertFalse(Assert.java:74)
        at org.apache.jmeter.protocol.http.sampler.TestHTTPSamplers.testRawBodyFromFile(TestHTTPSamplers.java:385)

and it provides no clue on what really went wrong.

It would be so much better if you used

String msg = sample.getResponseDataAsString();
if (msg.contains("java.io.FileNotFoundException:")) {
  fail("sample.getResponseDataAsString()"+
      " should not contain java.io.FileNotFoundException,"+
      " but the actual value is " + msg);
}

Do you see? Thank you for the understanding.

Copy link
Author

@artem-fedorov artem-fedorov Feb 28, 2019

@vlsi I'm agree with your remark. The second code style is better for understanding in fail cases.

Copy link
Contributor

@FSchumacher FSchumacher Feb 28, 2019

Thats is definitely better, but I think it would be even better to use hamcrests StringContains.constainsString and junits Assert.assertThat for the test.

Using static imports it would result in

assertThat(sample.getResponseDataAsString(), not(containsString("java.io.FileNotFoundException:")));

Copy link
Collaborator

@vlsi vlsi Feb 28, 2019

Mr @pmouawad might argue that Groovy+Spock might even be better (e.g. https://twitter.com/VladimirSitnikv/status/1100666414548045825 ).

However please estimate how assert failures would look like. Would it look like a good bug report? Is the failure message+stacktrace enough to file a understandable bug report?

What we typically see is that "tests pass". We don't accept code changes if tests fail, do we?

However the important bit of the test code is the way it fails should something wrong happen. It is important that developers understand that, and committers acting as gatekeepers should watch for that as well.

I don't mean "assert messages" are the top priority, however having sane messages is vital for further maintenance of the codebase.

Copy link
Contributor

@pmouawad pmouawad Feb 28, 2019

Mr @pmouawad might argue that Groovy+Spock might even be better (e.g. https://twitter.com/VladimirSitnikv/status/1100666414548045825 ).

I don't know if it's sarcastic or not :-)
Still, I find the messages very clear and text comparison a great improvement.
Integration with Eclipse is not that great (at least from my experience) but still interesting framework (Thanks Graham Russell (to Caesar what's belong to Caesar) for making me discover it). No firm conviction on JUnit 5 or Spock still, both do the job

Copy link
Contributor

@FSchumacher FSchumacher Feb 28, 2019

I like the spock error messages, too. They might be a bit over the top and frightening to the beginner, but very descriptive and therefore helpful.

... Would it look like a good bug report? Is the failure message+stacktrace enough to file a understandable bug report?

The error message would look like:

Expected: not a string containing "java.io.FileNotFoundException"
  but was: "the real content of the sampler ..."

In my opinion this is a good (enough) message, with the plus side, that is is code only and thus better suited for refactoring. But either way, we should get rid of simple assertFalse or assertTrue statements, that have no meaningful description message.

What we typically see is that "tests pass". We don't accept code changes if tests fail, do we?

What do you mean by this? If the code is wrong, or the error messages are difficult to read, we should -- and have done -- change the tests.

All in all, this seems to grow into a discussion, that would be good to have on the mailing list.

asfgit pushed a commit that referenced this issue Mar 2, 2019
The test used the setting of CWD, which is different when run from an IDE or from ant.
Now it uses JMeterHome which is the same for both environments.

And while we are at the test,
 * we try to make sure, that we don't disturb other tests with the manipulated base dir
 * use more descriptive messages in the case of a failure

Relates to #400 on github
Bugzilla Id: 62785



git-svn-id: https://svn.apache.org/repos/asf/jmeter/trunk@1854657 13f79535-47bb-0310-9956-ffa450edef68
asfgit pushed a commit that referenced this issue Mar 2, 2019
Relates to #400 on github
Bugzilla Id: 62785



git-svn-id: https://svn.apache.org/repos/asf/jmeter/trunk@1854660 13f79535-47bb-0310-9956-ffa450edef68
StorDm pushed a commit to etnetera/jmeter that referenced this issue Jan 7, 2021
…nctionality of the HTTP sampler

Implemented by Artem Fedorov (artem.fedorov at blazemeter.com) and contributed by BlazeMeter.

Closes apache#400 on github
Bugzilla Id: 62785


git-svn-id: https://svn.apache.org/repos/asf/jmeter/trunk@1842697 13f79535-47bb-0310-9956-ffa450edef68

Former-commit-id: 86d466b
StorDm pushed a commit to etnetera/jmeter that referenced this issue Jan 7, 2021
The test used the setting of CWD, which is different when run from an IDE or from ant.
Now it uses JMeterHome which is the same for both environments.

And while we are at the test,
 * we try to make sure, that we don't disturb other tests with the manipulated base dir
 * use more descriptive messages in the case of a failure

Relates to apache#400 on github
Bugzilla Id: 62785



git-svn-id: https://svn.apache.org/repos/asf/jmeter/trunk@1854657 13f79535-47bb-0310-9956-ffa450edef68

Former-commit-id: 3a8817f
StorDm pushed a commit to etnetera/jmeter that referenced this issue Jan 7, 2021
Relates to apache#400 on github
Bugzilla Id: 62785



git-svn-id: https://svn.apache.org/repos/asf/jmeter/trunk@1854660 13f79535-47bb-0310-9956-ffa450edef68

Former-commit-id: 2e1ea52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants