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

Jump to bottom

[JENKINS-48407] Re-enable test #3275

Merged
merged 4 commits into from Feb 2, 2018
Merged

[JENKINS-48407] Re-enable test #3275

merged 4 commits into from Feb 2, 2018

Conversation

batmat
Copy link
Member

@batmat batmat commented Jan 30, 2018 •
edited

Followup of #3274

The previous test assumed permissions would always be the same, when they actually depend on umask settings.

This change creates a file not using the temporary API, gets its permissions then compares it to the ones obtained using AtomicFileWriter.

Note: we now only check the given permissions, not the "non-given".

See JENKINS-48407.

Proposed changelog entries

N/A: changes only under src/test.

  • JIRA issue is well described
  • Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    * Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
  • Appropriate autotests or explanation to why this change has no tests
  • For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@reviewbybees esp. @daniel-beck

@batmat
[JENKINS-48407] Re-enable test
dcffb85 
The previous test assumed permissions would always be the same,
when they actually depend on umask settings.

This change creates a file *not* using the temporary API, gets its
permissions then compares it to the ones obtained using
AtomicFileWriter.

Note: we now only check the given permissions, not the "non-given".
dwnusbaum
dwnusbaum approved these changes Jan 30, 2018
View reviewed changes
Copy link
Member

@dwnusbaum dwnusbaum left a comment •
edited

Choose a reason for hiding this comment

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

🐝 I validated the test on my machine with a umask of 022 and 002. My only suggestion is that it might be better to directly assert that the expected and actual permissions are equal.

core/src/test/java/hudson/util/AtomicFileWriterTest.java Outdated
import static java.nio.file.attribute.PosixFilePermission.OTHERS_READ;
import static java.nio.file.attribute.PosixFilePermission.OTHERS_WRITE;
import static java.nio.file.attribute.PosixFilePermission.OWNER_EXECUTE;
import static java.nio.file.attribute.PosixFilePermission.OWNER_READ;
import static java.nio.file.attribute.PosixFilePermission.OWNER_WRITE;
Copy link
Member

Choose a reason for hiding this comment

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

nit: I think all of the static imports from java.nio.file.attribute.PosixFilePermission.* are unused.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

core/src/test/java/hudson/util/AtomicFileWriterTest.java Outdated
for (PosixFilePermission perm : notGivenPermissions) {
assertFalse("should not be allowed: " + perm, posixFilePermissions.contains(perm));
for (PosixFilePermission perm : DEFAULT_GIVEN_PERMISSIONS) {
assertTrue("missing expected permission: " + perm, posixFilePermissions.contains(perm));
Copy link
Member

Choose a reason for hiding this comment

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

Maybe replace the loop with assertThat(posixFilePermissions, equalTo(DEFAULT_GIVEN_PERMISSIONS)) to catch any changes that add extra permissions as well? You would also get nicely formatted failures for free:

Expected: <[OWNER_READ, GROUP_READ, OTHERS_READ, OWNER_WRITE]>
     but: was <[OWNER_READ, OTHERS_WRITE, GROUP_READ, OTHERS_READ, OWNER_WRITE]>

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

Thanks @dwnusbaum !

@batmat batmat mentioned this pull request Jan 30, 2018
Merged
4 tasks
svanoort
svanoort approved these changes Jan 30, 2018
View reviewed changes
Copy link
Member

@svanoort svanoort left a comment

Choose a reason for hiding this comment

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

Hearty upvote!

oleg-nenashev
oleg-nenashev approved these changes Jan 31, 2018
View reviewed changes
Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

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

🐝 and @reviewbybees done

@oleg-nenashev oleg-nenashev added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Jan 31, 2018
jtnord
jtnord reviewed Jan 31, 2018
View reviewed changes
core/src/test/java/hudson/util/AtomicFileWriterTest.java Outdated
@Rule
public TemporaryFolder tmp = new TemporaryFolder();
File af;
AtomicFileWriter afw;
String expectedContent = "hello world";

@BeforeClass
public static void computePermissions() throws IOException {
final File tempDir = com.google.common.io.Files.createTempDir();
Copy link
Member

Choose a reason for hiding this comment

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

why are you introducing this - you have TemporaryFolder which can create directories...

jtnord
jtnord requested changes Jan 31, 2018
View reviewed changes
core/src/test/java/hudson/util/AtomicFileWriterTest.java
if (!isPosixSupported(newFile)) {
return;
}
DEFAULT_GIVEN_PERMISSIONS = Files.getPosixFilePermissions(newFile.toPath());
Copy link
Member

Choose a reason for hiding this comment

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

🐛 the file and directory are not deleted

Copy link
Member Author

Choose a reason for hiding this comment

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

Not really a bug, granted the tmp directory could grow, but well it's not even production code. Using TemporaryFolder instead, constantly forget about this one.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jtnord fixed in last commit.

@batmat batmat requested a review from jtnord January 31, 2018 13:57
@oleg-nenashev oleg-nenashev merged commit 6cefcf1 into jenkinsci:master Feb 2, 2018
@batmat batmat deleted the JENKINS-48407-reenable-test branch February 2, 2018 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jtnord jtnord jtnord approved these changes

@dwnusbaum dwnusbaum dwnusbaum approved these changes

@oleg-nenashev oleg-nenashev oleg-nenashev approved these changes

@svanoort svanoort svanoort approved these changes

@daniel-beck daniel-beck Awaiting requested review from daniel-beck

Assignees
No one assigned
Labels
ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback
Projects
None yet
Milestone
No milestone
5 participants
@batmat @jtnord @dwnusbaum @oleg-nenashev @svanoort