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

Improve testing for temp directories #5483

Merged
merged 26 commits into from
Apr 6, 2021
Merged

Improve testing for temp directories #5483

merged 26 commits into from
Apr 6, 2021

Conversation

lachlan-roberts
Copy link
Contributor

No description provided.

lachlan-roberts and others added 2 commits October 21, 2020 14:43
@lachlan-roberts
Add tests for the WebAppContext tmp directory
a4e71c2 
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@olamy
add more test on ServletContext.TEMPDIR
62b7134 
Signed-off-by: olivier lamy <oliver.lamy@gmail.com>
janbartel
janbartel requested changes Oct 21, 2020
View reviewed changes
Copy link
Contributor

@janbartel janbartel left a comment

Choose a reason for hiding this comment

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

Good start, needs a couple of additions/changes. I'd also like to see all of the temp dir tests in a single test file, not just the tests for ServletContext.TEMPDIR.

Good call on the french cheese ;)

jetty-webapp/src/test/java/org/eclipse/jetty/webapp/ServletContextTmpAttributeTest.java Outdated Show resolved Hide resolved
jetty-webapp/src/test/java/org/eclipse/jetty/webapp/ServletContextTmpAttributeTest.java Outdated Show resolved Hide resolved
jetty-webapp/src/test/java/org/eclipse/jetty/webapp/ServletContextTmpAttributeTest.java Outdated Show resolved Hide resolved
jetty-webapp/src/test/java/org/eclipse/jetty/webapp/ServletContextTmpAttributeTest.java Outdated
@Disabled("will fail if executed as root or super power user so Disabled it")
public void attributeWithInvalidPermissionStringValue() throws Exception
{
WebInfConfiguration webInfConfiguration = new WebInfConfiguration();
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't the test create a directory, then remove user read permission from it?

Copy link
Member

Choose a reason for hiding this comment

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

but then the current user/process won't be able to delete it

Copy link
Contributor

Choose a reason for hiding this comment

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

Once you've removed write permission, can't you put it back again before the test ends?

@lachlan-roberts
add testing for tmp dir after hot redeploy of context
531472f 
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@lachlan-roberts
Copy link
Contributor Author

@olamy there is a test failure for ServletContextTmpAttributeTest.attributeWithInvalidStringValue()

@olamy
Copy link
Member

olamy commented Oct 21, 2020

@lachlan-roberts yup I know it's because the docker image are run using root user. I'm changing that now but I had to change a bit our image to add a user see webtide/jenkins-slave@6834e91

@lachlan-roberts
Copy link
Contributor Author

@olamy should this one be passing now after those changes to the Dockerfile?

@olamy
Copy link
Member

olamy commented Nov 4, 2020

@lachlan-roberts sadly not... because if the image is not started with the root user testcontainers cannot use docker :(

@lachlan-roberts
disable failing attributeWithInvalidStringValue test
02b5142 
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@lachlan-roberts
fix checkstyle issues
cf8cab5 
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@lachlan-roberts
move webapp temp dir tests to single file
2964714 
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@janbartel
Copy link
Contributor

I think we have a problem with our CI test environment: we rely on tests to execute to verify our builds and releases. If we go around commenting out tests, we'll never know if the software works or not!

@lachlan-roberts
improve testing for webapp temp directories
34adb9b 
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
lachlan-roberts
lachlan-roberts commented Nov 12, 2020
View reviewed changes
Copy link
Contributor Author

@lachlan-roberts lachlan-roberts left a comment

Choose a reason for hiding this comment

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

@olamy I have made some cleanups and changes to the ServletContext.TEMPDIR portion of the tests.

Can you also look at these disabled tests, we should do something about them.

jetty-webapp/src/test/java/org/eclipse/jetty/webapp/TempDirTest.java Outdated Show resolved Hide resolved
jetty-webapp/src/test/java/org/eclipse/jetty/webapp/TempDirTest.java Outdated Show resolved Hide resolved
@lachlan-roberts
throw if tempDir attribute was an unknown type
1329115 
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@lachlan-roberts
Copy link
Contributor Author

There is apparently a checkstyle violation but I can't find it, not getting it when I build locally.

@janbartel
Copy link
Contributor

@lachlan-roberts maybe you need to merge from 9.4.x to fix the checkstyle error?

@lachlan-roberts
Copy link
Contributor Author

@olamy any idea why in DeploymentTempDirTest the WebAppProvider works when run locally but not on jenkins.

@olamy
increase timeout for CI
3eaf444 
Signed-off-by: olivier lamy <oliver.lamy@gmail.com>
@olamy
Copy link
Member

olamy commented Dec 29, 2020

@lachlan-roberts weird. I have increased a bit timeout to see if this solve it. I don't understand why we need 3 times webAppProvider.scan(); ?

@joakime
Copy link
Contributor

joakime commented Feb 11, 2021

This PR has copyright header issues.


[INFO] --- license-maven-plugin:3.0:check (check-java-headers) @ jetty-project ---
[INFO] Checking licenses...
[WARNING] Missing header in: PR-5483/jetty-webapp/src/test/java/org/eclipse/jetty/webapp/TempDirTest.java
[WARNING] Missing header in: PR-5483/jetty-deploy/src/test/java/org/eclipse/jetty/deploy/DeploymentTempDirTest.java

@olamy
enable again test to try debug the issue
a3aa892 
Signed-off-by: olivier lamy <oliver.lamy@gmail.com>
@olamy
add some debug informations
f0462e9 
Signed-off-by: olivier lamy <oliver.lamy@gmail.com>
@olamy
more debug
cbb4803 
Signed-off-by: olivier lamy <oliver.lamy@gmail.com>
olamy and others added 5 commits March 2, 2021 20:24
@olamy
more debug
bf2ffa3 
Signed-off-by: olivier lamy <oliver.lamy@gmail.com>
@olamy
fix logging debug
fa3e52a 
Signed-off-by: olivier lamy <oliver.lamy@gmail.com>
@lachlan-roberts
Fix bug in DeploymentTempDirTest.
c968321 
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@lachlan-roberts
Merge remote-tracking branch 'origin/jetty-9.4.x' into jetty-9.4.x-Te…
4f7fb5b 
…mpDirTests
@lachlan-roberts
Cleanup jetty-logging.properties file for jetty-deploy tests.
4ab3ae4 
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@lachlan-roberts lachlan-roberts marked this pull request as ready for review March 19, 2021 06:11
janbartel
janbartel requested changes Mar 22, 2021
View reviewed changes
jetty-deploy/src/test/java/org/eclipse/jetty/deploy/DeploymentTempDirTest.java Show resolved Hide resolved
jetty-webapp/src/main/java/org/eclipse/jetty/webapp/WebInfConfiguration.java Show resolved Hide resolved
jetty-webapp/src/test/java/org/eclipse/jetty/webapp/TempDirTest.java Outdated Show resolved Hide resolved
jetty-webapp/src/test/java/org/eclipse/jetty/webapp/TempDirTest.java Show resolved Hide resolved
jetty-deploy/src/test/resources/jetty-logging.properties Show resolved Hide resolved
jetty-deploy/src/test/java/org/eclipse/jetty/deploy/DeploymentTempDirTest.java Show resolved Hide resolved
jetty-deploy/src/test/java/org/eclipse/jetty/deploy/DeploymentTempDirTest.java Show resolved Hide resolved
jetty-deploy/src/test/java/org/eclipse/jetty/deploy/DeploymentTempDirTest.java Outdated Show resolved Hide resolved
@lachlan-roberts
Changes from review.
6baa159 
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@lachlan-roberts
add tests for WebAppContext base tempDir
c5b9ff8 
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@lachlan-roberts
Remove validation of correct object type for temp directories.
a01da05 
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@lachlan-roberts
Add tests for jetty.base/work directory as temp directory.
7ea65d4 
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
janbartel
janbartel approved these changes Apr 1, 2021
View reviewed changes
Copy link
Contributor

@janbartel janbartel left a comment

Choose a reason for hiding this comment

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

LGTM

@lachlan-roberts lachlan-roberts merged commit 941d7fb into jetty-9.4.x Apr 6, 2021
@lachlan-roberts lachlan-roberts deleted the jetty-9.4.x-TempDirTests branch April 6, 2021 00:33
lachlan-roberts added a commit that referenced this pull request Apr 6, 2021
@lachlan-roberts @olamy
Improve testing for temp directories (#5483)
08c9536 
Improve testing around WebAppContext temporary directories.

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Co-authored-by: olivier lamy <oliver.lamy@gmail.com>
@lachlan-roberts lachlan-roberts mentioned this pull request Apr 6, 2021
Merged
lachlan-roberts added a commit that referenced this pull request Apr 30, 2021
@lachlan-roberts @olamy
Improve testing for temp directories (#6139)
7555d03 
* Improve testing for temp directories (#5483)

Improve testing around WebAppContext temporary directories.

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
Co-authored-by: olivier lamy <oliver.lamy@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@olamy olamy olamy approved these changes

@janbartel janbartel janbartel approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@lachlan-roberts @olamy @janbartel @joakime