You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/07/26 01:45:18 UTC
[GitHub] [iceberg] stevenzwu opened a new pull request, #5353: Remove TestEnvironmentUtil#testEnvironmentSubstitution() as it is bui…
stevenzwu opened a new pull request, #5353:
URL: https://github.com/apache/iceberg/pull/5353
…ld env dependent. It will fail with ImmutableMap.of("user-test", System.getenv().get("USER")), if the build env doesn't contain USER env variable, which is the case for our internal build env.
java.lang.NullPointerException: null value in entry: user-test=null
at org.apache.iceberg.relocated.com.google.common.collect.CollectPreconditions.checkEntryNotNull(CollectPreconditions.java:33)
at org.apache.iceberg.relocated.com.google.common.collect.SingletonImmutableBiMap.<init>(SingletonImmutableBiMap.java:43)
at org.apache.iceberg.relocated.com.google.common.collect.ImmutableBiMap.of(ImmutableBiMap.java:81)
at org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap.of(ImmutableMap.java:126)
at org.apache.iceberg.util.TestEnvironmentUtil.testEnvironmentSubstitution(TestEnvironmentUtil.java:31)
Also this test only covers a trivial method
public static Map<String, String> resolveAll(Map<String, String> properties) {
return resolveAll(System.getenv(), properties);
}
the other non-public resolveAll is already covered by TestEnvironmentUtil#testMultipleEnvironmentSubstitutions().
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] stevenzwu commented on pull request #5353: Core: Remove TestEnvironmentUtil#testEnvironmentSubstitution() as it is bui…
Posted by GitBox <gi...@apache.org>.
stevenzwu commented on PR #5353:
URL: https://github.com/apache/iceberg/pull/5353#issuecomment-1195786021
@rdblue I thought about sth similar to your proposal earlier. We can make the test to assert on different expected output depends on the whether the `USER` env variable exists or not. But then I looked the public `resolveAll` method and found it is so trivial and simply calls the other non-public `resolveAll` method. And the non-public `resolveAll` is already covered by the unit test. Hence, I think it is not necessary to keep this test for a trivial method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue merged pull request #5353: Core: Remove TestEnvironmentUtil#testEnvironmentSubstitution() as it is bui…
Posted by GitBox <gi...@apache.org>.
rdblue merged PR #5353:
URL: https://github.com/apache/iceberg/pull/5353
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #5353: Core: Remove TestEnvironmentUtil#testEnvironmentSubstitution() as it is bui…
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #5353:
URL: https://github.com/apache/iceberg/pull/5353#issuecomment-1199951642
Thanks, @stevenzwu!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #5353: Core: Remove TestEnvironmentUtil#testEnvironmentSubstitution() as it is bui…
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #5353:
URL: https://github.com/apache/iceberg/pull/5353#issuecomment-1195729898
@stevenzwu, is it possible to fix this test instead? Can we ensure that a value is present for an environment variable? I think it is good to ensure that the properties are actually loaded from the environment.
Alternatively, maybe we could use something other than an `ImmutableMap` so that a `null` value is okay.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org