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.

Sign up for GitHub

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

Jump to bottom

Review disabled tests in Jetty 9.4.x #5684

Closed
janbartel opened this issue Nov 17, 2020 · 8 comments
Closed

Review disabled tests in Jetty 9.4.x #5684

janbartel opened this issue Nov 17, 2020 · 8 comments
Assignees
@gregw @joakime @sbordet
Labels
High Priority Stale For auto-closed stale issues and pull requests Test
Milestone
9.4.x

Comments

@janbartel
Copy link
Contributor

janbartel commented Nov 17, 2020 •
edited

We should only have very few disabled tests, and they should be disabled preferably with a condition.

See Issue #6327 for tests disabled in Jetty 10.0.x

This is the list of @Disabled for jetty-9.4.x:
@janbartel janbartel added this to the 9.4.x milestone Nov 17, 2020
@joakime
Copy link
Contributor

joakime commented Nov 17, 2020 •
edited

The @DisabledOn* are JVM / OS specific, and likely are sane decisions.
Also, don't forget there's also @EnabledOn* as well (which skip tests too).

I see a few @DisabledIf* annotations in this list, those are interesting (to me)

And there's also Assumptions that disable tests based on runtime criteria as well. (a JVM that doesn't support ALPN for example)

@joakime
Copy link
Contributor

joakime commented Nov 17, 2020

You can use the "skipped" column (sort by it) on the latest test run to know which modules to focus first on.

Example: https://jenkins.webtide.net/job/jetty.project/job/jetty-9.4.x/1863/testReport/ (click the "Skip" column heading once)

@joakime
Copy link
Contributor

joakime commented Nov 17, 2020

@janbartel I edited your issue to make it a task list that any of us can check off when we've reviewed those @disabled entries.

@joakime
Copy link
Contributor

joakime commented Nov 17, 2020

The ones labelled @DisabledOnJre({JRE.JAVA_8, JRE.JAVA_9, JRE.JAVA_10}) are basically JRE 11+
Those look sane to me.

@janbartel
Copy link
Contributor Author

@joakime after fixing PathWatcher in #5830, can you please check if it is still necessary to keep the PathWatcherTest disabled?? I ran it (on linux) without problems.

janbartel added a commit that referenced this issue Jan 12, 2021
@janbartel
Issue #5684 Fix testSetBadBoolean and remove @disabled
5212422 
Signed-off-by: Jan Bartel <janb@webtide.com>
@janbartel janbartel mentioned this issue Jan 12, 2021
Merged
janbartel added a commit that referenced this issue Jan 12, 2021
@janbartel
Issue #5684 Fix testSetBadBoolean and remove @disabled (#5875)
a5a8327 
Signed-off-by: Jan Bartel <janb@webtide.com>
janbartel added a commit that referenced this issue Jan 13, 2021
@janbartel
Issue #5684 Fix and re-enable ServletRequestLogTest.
995b08a 
Signed-off-by: Jan Bartel <janb@webtide.com>
@janbartel janbartel mentioned this issue Jan 13, 2021
Merged
janbartel added a commit that referenced this issue Jan 13, 2021
@janbartel
Issue #5684 Fix and re-enable ServletRequestLogTest. (#5877)
b99eb3f 
Signed-off-by: Jan Bartel <janb@webtide.com>
@gregw
Copy link
Contributor

gregw commented Mar 22, 2021

@joakime @sbordet @gregw I've assigned this issue to all of us. We need to address this list by fixing/reviewing tests in our areas and once done, remove yourself from the assigned list.
Last person to do so, get's left with all the left over ones to fix!!!

gregw added a commit that referenced this issue Mar 22, 2021
@gregw
Fixes for #5684
da8a19c 
Simplified CyclicTimeoutTest#testBusy

Signed-off-by: Greg Wilkins <gregw@webtide.com>
gregw added a commit that referenced this issue Mar 22, 2021
@gregw
Fixes for #5684
e072f1e 
InclusiveByteRange clears range list on errors
InclusiveByteRange is forgiving of tab separators and leading 0s

Signed-off-by: Greg Wilkins <gregw@webtide.com>
@gregw gregw mentioned this issue Mar 22, 2021
Merged
gregw added a commit that referenced this issue Mar 30, 2021
@gregw
Jetty 10.0.x #5684 disabled tests (#6081)
20fae64 
* Fixes for #5684
Simplified CyclicTimeoutTest#testBusy
InclusiveByteRange clears range list on errors
InclusiveByteRange is forgiving of tab separators and leading 0s

Signed-off-by: Greg Wilkins <gregw@webtide.com>
janbartel added a commit that referenced this issue Apr 7, 2021
@janbartel
Issue #5684 Reduce intensity and rename SameNodeLoadTest
0380576 
Signed-off-by: Jan Bartel <janb@webtide.com>
@joakime joakime mentioned this issue Apr 12, 2021
Closed
janbartel added a commit that referenced this issue Apr 13, 2021
@janbartel
Issue #5684 Reduce intensity and rename SameNodeLoadTest (#6143)
57d0bae 
Signed-off-by: Jan Bartel <janb@webtide.com>
janbartel added a commit that referenced this issue Apr 13, 2021
@janbartel
Issue #5684 Reduce intensity and rename SameNodeLoadTest (#6143)
694017d 
Signed-off-by: Jan Bartel <janb@webtide.com>
@sbordet sbordet added this to To do in Jetty 9.4.40 via automation Apr 14, 2021
@sbordet sbordet moved this from To do to Done in Jetty 9.4.40 Apr 14, 2021
@sbordet sbordet removed this from the 9.4.x milestone Apr 14, 2021
@joakime joakime changed the title Review disabled tests Review disabled tests in Jetty 9.4.x May 27, 2021
@joakime joakime added the Test label Jun 8, 2021
joakime added a commit that referenced this issue Jun 16, 2021
@joakime
Issue #5684 - more disabled test cleanup
5d7cbff 
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
joakime added a commit that referenced this issue Jun 18, 2021
@joakime
Issue #5684 - more disabled test cleanup
704abc6 
+ Assumption based on existence of
  possible DNS Hijacking
+ Alternate logic for client side
  protocol and cipher suite mismatch
  behavior on server side based
  on client side protocol existence
  of TLSv1.3

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
joakime added a commit that referenced this issue Jun 18, 2021
@joakime
Issue #5684 - Improve warning about DNS Hijacking during testing.
bb24f0f 
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
joakime added a commit that referenced this issue Jul 23, 2021
@joakime
Merge pull request #6419 from eclipse/jetty-9.4.x-remove-disabled-tls…
cd20770 
…-tests

Issue #5684 - Client and HttpServerTestBase disabled test cleanup
joakime added a commit that referenced this issue Jul 23, 2021
@joakime
Issue #5684 - Update TLS tests
713136f 
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
@lorban lorban mentioned this issue Jul 26, 2021
Closed
joakime added a commit that referenced this issue Jul 27, 2021
@joakime
Merge pull request #6537 from eclipse/jetty-10.0.x-tls-test-revamp
330fc0b 
Issue #5684 - Update TLS tests
joakime added a commit that referenced this issue Jul 27, 2021
@joakime
Issue #5684 - Window's test overhaul
7741ecc 
+ Migrate from @DisabledOnOs(WINDOWS) to assumptions on capabilities instead.
+ Fix other outstanding windows testing issues.

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
joakime added a commit that referenced this issue Jul 30, 2021
@joakime
Issue #5684 - Window's test overhaul
87912d8 
+ Cleanup FileBufferedResponseHandlerTest expectations on Windows.

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
joakime added a commit that referenced this issue Aug 2, 2021
@joakime
Issue #5684 - Restrict behaviors to TLSv1.2
fb1c854 
+ Windows TLS behaviors between
  OpenJDK 8 and OpendJDK 11
  and even between TLS versions
  make the test unreliable.

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
joakime added a commit that referenced this issue Aug 2, 2021
@joakime
Issue #5684 - scan interval is variable on windows
500e6f2 
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
joakime added a commit that referenced this issue Aug 2, 2021
@joakime
Issue #5684 - if unable to start, the stop shouldn't fail
75c7ce2 
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
joakime added a commit that referenced this issue Aug 2, 2021
@joakime
Issue #5684 - increase timeout
93e47a6 
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
joakime added a commit that referenced this issue Aug 2, 2021
@joakime
Issue #5684 - make test less strict due to system speed issues
18f32da 
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
joakime added a commit that referenced this issue Aug 2, 2021
@joakime
Issue #5684 - Disabled TLSv1.3 tests on windows
60bfc75 
If we restrict to TLSv1.2 this passes.

But on TLSv1.3 is a behavior differences between Linux and Windows.

On Linux TLSv.13 on client side will always return a
javax.net.ssl.SSLHandshakeException in those test cases that expect it.

However, on Windows, Only the TLSv1.2 implementation will return a javax.net.ssl.SSLHandshakeException,

All other TLS versions will result in a
javax.net.ssl.SSLException: Software caused connection abort: recv failed

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
joakime added a commit that referenced this issue Aug 2, 2021
@joakime
Issue #5684 - Disable ConcurrentStreamCreationTest
1c16aa8 
+ Not possible to create all of these streams.

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
joakime added a commit that referenced this issue Aug 2, 2021
@joakime
Issue #5684 - Fixing DeploymentTempDirTest
242f662 
Using unique workdir per testcase.
Don't expect to delete between tests (not supported on windows due to file locking anyway)

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
joakime added a commit that referenced this issue Aug 3, 2021
@joakime
Merge pull request #6572 from eclipse/jetty-9.4.x-windows-test-overhaul
51e6335 
Issue #5684 - windows test overhaul
joakime added a commit that referenced this issue Aug 3, 2021
@joakime
Issue #5684 - Window's test overhaul
0234f59 
+ Migrate from @DisabledOnOs(WINDOWS) to assumptions on capabilities instead.
+ Fix other outstanding windows testing issues.
+ Cleanup FileBufferedResponseHandlerTest expectations on Windows.
+ PathWatcher scan interval is variable on windows
+ If unable to start testcase based on assumption,
  the stop shouldn't fail testcase
+ Increase various wait timeouts
+ Make tests less strict due to system speed issues
+ Disable Sni tests due to TLS behaviors differences in Windows
  + Windows TLSv1.3 seems to introduce this difference
  + If we restrict to TLSv1.2 this passes.
  + On Linux TLSv.13 on client side will always return a
  + javax.net.ssl.SSLHandshakeException in those test cases that expect it.
  + However, on Windows, Only the TLSv1.2 implementation will return a javax.net.ssl.SSLHandshakeException,
  + All other TLS versions on Windows will result in a
  + javax.net.ssl.SSLException: Software caused connection abort: recv failed
+ Disable ConcurrentStreamCreationTest
  + Not possible to create all of these streams.
+ Fixing DeploymentTempDirTest
  + Using unique workdir per testcase.
  + Don't expect to delete files / directories between tests
    (not supported on windows due to file locking anyway)

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
joakime added a commit that referenced this issue Aug 3, 2021
@joakime
Issue #5684 - Fixing compilation error
fe61cab 
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
joakime added a commit that referenced this issue Aug 4, 2021
@joakime
Issue #5684 - Fixing regex for pathsep
df26fe6 
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
joakime added a commit that referenced this issue Aug 5, 2021
@joakime
Issue #5684 - Fix and re-enable RFC2616NIOHttpsTest
ca20308 
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
joakime added a commit that referenced this issue Aug 26, 2021
@joakime @olamy
Issue #5684 - Windows test overhaul (#6581)
c5a33e2 
Issue #5684 - Window's test overhaul

+ Migrate from @DisabledOnOs(WINDOWS) to assumptions on capabilities instead.
+ Fix other outstanding windows testing issues.
+ Cleanup FileBufferedResponseHandlerTest expectations on Windows.
+ PathWatcher scan interval is variable on windows
+ If unable to start testcase based on assumption,
  the stop shouldn't fail testcase
+ Increase various wait timeouts
+ Make tests less strict due to system speed issues
+ Disable Sni tests due to TLS behaviors differences in Windows
  + Windows TLSv1.3 seems to introduce this difference
  + If we restrict to TLSv1.2 this passes.
  + On Linux TLSv.13 on client side will always return a
  + javax.net.ssl.SSLHandshakeException in those test cases that expect it.
  + However, on Windows, Only the TLSv1.2 implementation will return a javax.net.ssl.SSLHandshakeException,
  + All other TLS versions on Windows will result in a
  + javax.net.ssl.SSLException: Software caused connection abort: recv failed
+ Disable ConcurrentStreamCreationTest
  + Not possible to create all of these streams.
+ Fixing DeploymentTempDirTest
  + Using unique workdir per testcase.
  + Don't expect to delete files / directories between tests
    (not supported on windows due to file locking anyway)
 + Fixing line ending difference on windows
 + InvalidPathException is a 404 Not Found
 + Cannot reuse test directory between runs due to memory mapped files that are still in use from previous run.
 + java.nio.file.FileSystemException: C:\code\jetty.project\jetty-webapp\target\tests\welcome#\index.html: The requested operation cannot be performed on a file with a user-mapped section open.

	at java.base/sun.nio.fs.WindowsException.translateToIOException(WindowsException.java:92)
	at java.base/sun.nio.fs.WindowsException.rethrowAsIOException(WindowsException.java:103)
	at java.base/sun.nio.fs.WindowsException.rethrowAsIOException(WindowsException.java:108)
	at java.base/sun.nio.fs.WindowsFileSystemProvider.newByteChannel(WindowsFileSystemProvider.java:235)
	at java.base/java.nio.file.spi.FileSystemProvider.newOutputStream(FileSystemProvider.java:478)
	at java.base/java.nio.file.Files.newOutputStream(Files.java:220)
	at org.eclipse.jetty.webapp/org.eclipse.jetty.webapp.WebAppDefaultServletTest.prepareServer(WebAppDefaultServletTest.java:84)

 + As is typical on windows, we are often unable to delete a file due to file locking issues.
 + Use a unique resource base between tests.
   This is to avoid file locking behaviors that prevent the
   resource base from being reused too quickly on windows.
 + Prevent test run if symlinks not supported
 + Allowing for Windows slosh char as well in asserts
 + SelectorUtils is File.separator dependent
 + Regex is now FS.separator independent
 + Using SelectorUtils from plexus correctly for include/exclude
 + Turning off mapped files for testing reasons.
 + Fix and re-enable RFC2616NIOHttpsTest
 + Issue #6552 - Fix test failures due to slf4j dep
 + Issue #6552 - upgrade testcontainers
 + Issue #6552 - move to assumption based docker existence
 + Issue #6552 - Fix enforcer rule violation on jna.
  Addresses the following side effect of upgrading testcontainers.

[WARNING] Rule 3: org.apache.maven.plugins.enforcer.RequireUpperBoundDeps failed with message:
Failed while enforcing RequireUpperBoundDeps. The error(s) are [
Require upper bound dependencies error for net.java.dev.jna:jna:5.6.0 paths to dependency are:
+-org.eclipse.jetty:infinispan-remote-query:10.0.7-SNAPSHOT
  +-org.testcontainers:testcontainers:1.16.0
    +-com.github.docker-java:docker-java-transport-zerodep:3.2.11
      +-net.java.dev.jna:jna:5.6.0 (managed) <-- net.java.dev.jna:jna:5.8.0

 + use annotation to disable test when docker not available and needed
 + Disabling FileSessionDistributionTests.stopRestartWebappTestSessionContentSaved on Windows
 + Using TLS basic
 + Programmatic removal of memory mapped behavior during testing
 + Fixing slf4j warning

Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
Co-authored-by: Olivier Lamy <oliver.lamy@gmail.com>
@janbartel janbartel mentioned this issue Sep 2, 2021
Closed
@github-actions
Copy link

This issue has been automatically marked as stale because it has been a
full year without activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@github-actions github-actions bot added the Stale For auto-closed stale issues and pull requests label Aug 19, 2022
@github-actions
Copy link

This issue has been closed due to it having no activity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@gregw gregw

@joakime joakime

@sbordet sbordet

Labels
High Priority Stale For auto-closed stale issues and pull requests Test
Projects
None yet
Milestone
9.4.x
Development

No branches or pull requests
4 participants
@gregw @joakime @janbartel @sbordet