Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 353509

Summary: jetty-client unit tests are running too long
Product: [RT] Jetty Reporter: Thomas Becker <tbecker>
Component: clientAssignee: Joakim Erdfelt <joakim.erdfelt>
Status: RESOLVED FIXED QA Contact:
Severity: minor    
Priority: P3 CC: jetty-inbox, joakim.erdfelt, mgorovoy
Version: 7.4.2   
Target Milestone: 7.5.x   
Hardware: PC   
OS: Mac OS X - Carbon (unsup.)   
Whiteboard:
Attachments:
Description Flags
patch with refactored tests
none
Fix of issues after merge none

Description Thomas Becker CLA 2011-08-01 11:34:17 EDT 
Build Identifier: 

Some of the jetty client unit tests are running too long. The patch attached will shorten the runtime of the tests significantly by reusing server and client. The drawback is that the tests will be coupled together a bit more, but this shouldn't cause any issues and the runtime benefits will overcompensate the drawback.

Reproducible: Always
Comment 1 Thomas Becker CLA 2011-08-01 11:38:54 EDT 
Created attachment 200662 [details]
patch with refactored tests
Comment 2 Thomas Becker CLA 2011-08-01 11:39:23 EDT 
*** Bug 352991 has been marked as a duplicate of this bug. ***
Comment 3 Thomas Becker CLA 2011-08-02 04:49:27 EDT 
Runtime Changes of tests changed:

HttpExchangeTest:                         was: ~13s now: ~2.7s
AsyncSslHttpExchangeTest:                 was: ~10s now: ~2s
SslHttpExchangeTest:                      was: ~8.5s now: ~2s
ExternalKeyStoreAsyncSslHttpExchangeTest: was: ~8.3s now: ~2s 

Overall about 30s saved.
Comment 4 Michael Gorovoy CLA 2011-08-29 16:49:42 EDT 
Thomas,

I would suggest not to move the code that creates the server and client objects to a separate set of classes. It is unlikely that we will be able to reuse the code that creates these objects because it is often unique to each test. Yet it makes the test code even less readable when it is right now.

The test performance improvements in the current patch are obscured by the re-factoring and re-formatting changes, so in my opinion it would be best if we commit the changes that affect test performance separately, and then decide if further re-factoring of the tests is needed.

Thanks,
Michael
Comment 5 Joakim Erdfelt CLA 2011-09-12 17:39:19 EDT 
The patch no longer applies.
But I can see what you were doing.
So I'll manually merge it.
Comment 6 Joakim Erdfelt CLA 2011-09-12 18:07:35 EDT 
Have finished manually merging this patch.
It doesn't run/test successfully tho.

See new branch 'bug-353509' for details.

$ git branch --track bug-353509 refs/remotes/origin/bug-353509
Comment 7 Thomas Becker CLA 2011-09-14 04:46:07 EDT 
Created attachment 203320 [details]
Fix of issues after merge

Hi Joakim,

attached a first patch fixing some merging issues. Basically you forgot to merge few lines of code. These are fixed now.

There's one problem left breaking the "testOptionsWithExchange" test in all 4 variants of the unit test:

  testOptionsWithExchange(org.eclipse.jetty.client.AsyncSslHttpExchangeTest): Response does not contain Allow header
  testOptionsWithExchange(org.eclipse.jetty.client.ExternalKeyStoreAsyncSslHttpExchangeTest): Response does not contain Allow header
  testOptionsWithExchange(org.eclipse.jetty.client.HttpExchangeTest): Response does not contain Allow header
  testOptionsWithExchange(org.eclipse.jetty.client.SslHttpExchangeTest): Response does not contain Allow header

I will dig deeper into that one in my afternoon after aligning with you.

Cheers,
Thomas
Comment 8 Joakim Erdfelt CLA 2011-09-15 15:13:49 EDT 
Considering this fixed.
Not much more test timing can be eked out of jetty-client.
Comment 9 Joakim Erdfelt CLA 2011-09-15 15:14:10 EDT 
Setting to fixed.