liamzwbao commented on code in PR #1126: URL: https://github.com/apache/polaris/pull/1126#discussion_r2017784576
########## quarkus/service/src/testFixtures/java/org/apache/polaris/service/quarkus/it/PolarisQuarkusTestBeforeClassCallback.java: ########## @@ -0,0 +1,32 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.polaris.service.quarkus.it; + +import io.quarkus.test.junit.callback.QuarkusTestBeforeClassCallback; +import org.assertj.core.api.Assumptions; +import org.assertj.core.configuration.PreferredAssumptionException; + +public class PolarisQuarkusTestBeforeClassCallback implements QuarkusTestBeforeClassCallback { + @Override + public void beforeClass(Class<?> testClass) { Review Comment: > This will be called for every class, right? Yes, exactly. This hook behaves like a @BeforeAll for all test classes. > Is there a way to make one call to Assumptions.setPreferredAssumptionException() is each test JVM? > We could put the override in a static block on the common / shared test class... WDYT? I tried adding a static block inside `PolarisIntegrationTestExtension`, since many test classes extend it, but it didn’t seem to take effect, so it might not be the right place for it. Do you have any thoughts on where a static block like this would reliably run once per test JVM? > Sorry, if my previous comments made you do extra work 😅 I was only wondering what was possible. No worries at all. Consolidating the config for all tests is definitely better than setting it individually, and digging into how Quarkus callbacks work turned out to be a good learning opportunity. > What it actually broken without override assumption defaults? From what I’ve seen, it’s related to Iceberg still having some tests written in JUnit 4. When an assumption fails using AssertJ, the exception differs: - JUnit 4 throws `AssumptionViolatedException` - JUnit 5 throws `TestAbortedException` If we’re purely using JUnit 5, this isn’t a problem. However, when tests are annotated with @QuarkusTest, the JUnit 4-style exceptions don’t get handled properly: instead of skipping the test, Quarkus treats it as a failure. I did some digging into AssertJ, and overriding the assumption exception type is the recommended workaround for this issue. You could refer to quarkusio/quarkus#46642 and assertj/assertj#2267 for more info. -- 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-unsubscr...@polaris.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
