You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Ignasi Barrera <no...@github.com> on 2017/10/23 20:42:02 UTC
[jclouds/jclouds] Exclude tier tests on OSX (#1150)
You can view, comment on, or merge this pull request online at:
https://github.com/jclouds/jclouds/pull/1150
-- Commit Summary --
* Exclude tier tests on OSX
-- File Changes --
M apis/filesystem/src/test/java/org/jclouds/filesystem/integration/FilesystemBlobIntegrationTest.java (11)
M blobstore/src/test/java/org/jclouds/blobstore/integration/internal/BaseBlobIntegrationTest.java (2)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/1150.patch
https://github.com/jclouds/jclouds/pull/1150.diff
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1150
Re: [jclouds/jclouds] Exclude tier tests on OSX (#1150)
Posted by Andrew Phillips <no...@github.com>.
demobox commented on this pull request.
> }
@Override
public void testSetBlobAccess() throws Exception {
throw new SkipException("filesystem does not support anonymous access");
}
+
+ private void checkExtendedAttributesSupport() {
[minor] Does this need to be an instance method, or could this be static?
--
You are receiving this because you commented.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1150#pullrequestreview-71471307
Re: [jclouds/jclouds] Exclude tier tests on OSX (#1150)
Posted by Ignasi Barrera <ig...@gmail.com>.
I followed the same approach than the other methods in this class. Should
we better change them all to throw skip exceptions?
On Monday, 23 October 2017, Andrew Gaul <no...@github.com> wrote:
> *@gaul* requested changes on this pull request.
> ------------------------------
>
> In apis/filesystem/src/test/java/org/jclouds/filesystem/integration/
> FilesystemBlobIntegrationTest.java
> <https://github.com/jclouds/jclouds/pull/1150#discussion_r146391479>:
>
> > @@ -89,6 +91,15 @@ protected void checkUserMetadata(Map<String, String> userMetadata1, Map<String,
> }
> }
>
> + /* Java on OS X does not support extended attributes, which the filesystem backend
> + * uses to implement storage tiers */
> + @Override
> + protected void testPutBlobTierHelper(Tier tier, PutOptions options) throws Exception {
> + if (!isMacOSX()) {
>
> Instead could you flag this as skipped via:
>
> if (isMacOSX()) {
> throw new SkipException("Extended attributes not supported on Mac OS X");
> }
>
> —
> You are receiving this because you are subscribed to this thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/jclouds/jclouds/pull/1150#pullrequestreview-71327087>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AEN2-uNCnZifF66f10L_NRqcc1pnFWFrks5svP2YgaJpZM4QDduN>
> .
>
Re: [jclouds/jclouds] Exclude tier tests on OSX (#1150)
Posted by Andrew Gaul <no...@github.com>.
gaul requested changes on this pull request.
> @@ -89,6 +91,15 @@ protected void checkUserMetadata(Map<String, String> userMetadata1, Map<String,
}
}
+ /* Java on OS X does not support extended attributes, which the filesystem backend
+ * uses to implement storage tiers */
+ @Override
+ protected void testPutBlobTierHelper(Tier tier, PutOptions options) throws Exception {
+ if (!isMacOSX()) {
Instead could you flag this as skipped via:
```java
if (isMacOSX()) {
throw new SkipException("Extended attributes not supported on Mac OS X");
}
```
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1150#pullrequestreview-71327087
Re: [jclouds/jclouds] Exclude tier tests on OSX (#1150)
Posted by Ignasi Barrera <no...@github.com>.
Ok, I've cleaned up the commits and the code, leaving just the proper test skip, as the other methods were already ok.
--
You are receiving this because you commented.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1150#issuecomment-339019323
Re: [jclouds/jclouds] Exclude tier tests on OSX (#1150)
Posted by Ignasi Barrera <no...@github.com>.
> I think it is better to throw SkipException
Done!
--
You are receiving this because you commented.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1150#issuecomment-338887045
Re: [jclouds/jclouds] Exclude tier tests on OSX (#1150)
Posted by Ignasi Barrera <no...@github.com>.
@nacx pushed 1 commit.
6e5357f Throw skip exceptions instead of silently passing tests
--
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/jclouds/jclouds/pull/1150/files/76375fe35defe9b1d3b2732d143cfbdfde80801a..6e5357fc597e9f16ed27e7dbdfe32260c9730a1b
Re: [jclouds/jclouds] Exclude tier tests on OSX (#1150)
Posted by Andrew Gaul <no...@github.com>.
gaul requested changes on this pull request.
> }
// Mac OS X HFS+ does not support UserDefinedFileAttributeView:
// https://bugs.openjdk.java.net/browse/JDK-8030048
@Override
protected void validateMetadata(BlobMetadata metadata) throws IOException {
- if (!isMacOSX()) {
- super.validateMetadata(metadata);
- }
+ checkExtendedAttributesSupport();
This is what I mentioned in my previous review -- this will cause a test which checks many metadata, including non-xattr, to skip. We should only skip full tests.
> @Override
protected void checkUserMetadata(Map<String, String> userMetadata1, Map<String, String> userMetadata2) {
- if (!isMacOSX()) {
- super.checkUserMetadata(userMetadata1, userMetadata2);
- }
+ checkExtendedAttributesSupport();
+ super.checkUserMetadata(userMetadata1, userMetadata2);
+ }
+
+ /*
+ * Java on OS X does not support extended attributes, which the filesystem
+ * backend uses to implement storage tiers
+ */
+ @Override
+ protected void testPutBlobTierHelper(Tier tier, PutOptions options) throws Exception {
+ checkExtendedAttributesSupport();
+ super.testPutBlobTierHelper(tier, options);
This correctly skips.
> }
@Override
public void testSetBlobAccess() throws Exception {
throw new SkipException("filesystem does not support anonymous access");
}
+
+ private void checkExtendedAttributesSupport() {
+ if (isMacOSX()) {
+ throw new SkipException("filesystem does not support extended attributes on Mac OSX");
I don't think this adds anything an prevents `grep` from finding tests we skip, part of the expanded scope of this pull request.
--
You are receiving this because you commented.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1150#pullrequestreview-71534715
Re: [jclouds/jclouds] Exclude tier tests on OSX (#1150)
Posted by Andrew Gaul <no...@github.com>.
@nacx Agreed that this is inconsistent. I think it is better to throw `SkipException` since this flags tests in an obvious way, although several tests use `dataProvider` or conditional logic to avoid running some tests. If you do change this, note that methods like `validateMetadata` are not tests and need to conditionally skip Mac OS X so that other tests methods still run.
--
You are receiving this because you commented.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1150#issuecomment-338803163
Re: [jclouds/jclouds] Exclude tier tests on OSX (#1150)
Posted by Ignasi Barrera <no...@github.com>.
Merged to master as [f7b74d95](http://git-wip-us.apache.org/repos/asf/jclouds/commit/f7b74d95).
--
You are receiving this because you commented.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1150#issuecomment-339231822
Re: [jclouds/jclouds] Exclude tier tests on OSX (#1150)
Posted by Andrew Phillips <no...@github.com>.
demobox commented on this pull request.
> }
@Override
public void testSetBlobAccess() throws Exception {
throw new SkipException("filesystem does not support anonymous access");
}
+
+ private void checkExtendedAttributesSupport() {
+ if (isMacOSX()) {
+ throw new SkipException("filesystem does not support extended attributes on Mac OSX");
If we end up using `SkipException` in lots of places, thoughts on creating a `TestUtils.skip` helper for readability? Then we could write:
```
if (isMacOSX()) {
skip("filesystem does not support extended attributes on Mac OSX");
}
```
?
Thanks for taking care of this, @nacx!
--
You are receiving this because you commented.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1150#pullrequestreview-71472165
Re: [jclouds/jclouds] Exclude tier tests on OSX (#1150)
Posted by Ignasi Barrera <no...@github.com>.
Closed #1150.
--
You are receiving this because you commented.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1150#event-1309207208
Re: [jclouds/jclouds] Exclude tier tests on OSX (#1150)
Posted by Andrew Gaul <no...@github.com>.
gaul approved this pull request.
--
You are receiving this because you commented.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1150#pullrequestreview-71571973