Refactored test header logic into a global spock extension #22573

rnpmsft · 2021-06-25T17:23:33Z

Sooo this is one possible solution for centralizing the header logic, I've gone ahead and removed all header prints throughout the storage SDK in this branch as well. To deal with the classpath issue, I've changed the bundling for shared resources to use maven-assembly-plugin rather than the jar plugin; this way, we can have the META-INF folder somewhere that won't conflict, and redirect it within the jar.

The other solution was using maven-resources-plugin and maven-clean-plugin to move the extension manifest from some location into target/test-classes/META-INF/services right before the test-compile phase, then clean the file out right after (with process-test-classes), but the assembly sol seemed a bit tidier.

File locations and stuff could use some change, I just put everything somewhere it would work. assembly.xml/the manifests folder might do better in a centralized location, since they're only used by the shared build; maybe test-shared/resources (if that doesn't automatically get sent by maven to target/test-classes). We could also put the SpockConfig.groovy into there to separate shared tests resources fully from just common test resources.

One iffy thing in here; I nabbed the getTestName logic from StorageSpec and put it into the extensions for use with printing headers, and this caused code duplication; for now, there's a static reference from StorageSpec into the extension to call the method, which probably should be changed. Any thoughts on fixing that? Maybe just a TestNamer or something that both the spec and extension can use?

Azure#22430)

rnpmsft · 2021-06-25T17:25:47Z

I should've marked this as a draft; this likely isn't ready for merge before decoupling the extension and spec.

kasobol-msft · 2021-06-25T17:27:19Z

Did you just solve the META-INF conflict problem ?

kasobol-msft · 2021-06-25T17:39:51Z

...-storage-common/src/test-shared/java/com/azure/storage/common/test/shared/StorageSpec.groovy

@@ -19,10 +20,9 @@ class StorageSpec extends Specification {
    private StorageResourceNamer namer

    def setup() {
-        def testName = getTestName()
+        def testName = TestHeaderExtension.FeatureInterceptor.getTestName(specificationContext.getCurrentIteration())


we should create TestNameProvider or something like this and extract getTestName there and use it in both places.
Can be static code.

kasobol-msft · 2021-06-25T17:41:56Z

sdk/storage/azure-storage-common/pom.xml

+              <descriptors>
+                <descriptor>src/test-shared/assembly/assembly.xml</descriptor>
+              </descriptors>


how does the output of this look like in target folder when you run mvn install?

@alzimmermsft does this affect how commons is released to mvncentral anyhow that we should be worried about?

maven doesnt copy over the assembly folder/file into target since it's not specified as a resource. the manifests get transferred as a manifests folder into the target/test-classes folder, preventing it from getting caught with common tests, and the assembly plugin maps it where it needs to go in the artifact

yeah, but does test jar look the same ?

yeah, I ran jar xf on it and the contents are the same.

This shouldn't affect releasing to Maven central as this only affect building the library from source

alzimmermsft · 2021-06-25T17:43:09Z

Appears you've found a bug in the generate_from_source_pom.py script with the azure-storage-common self-reference, I'll submit a PR to fix this that'll need to be pulled into this branch.

kasobol-msft · 2021-06-25T17:46:42Z

Appears you've found a bug in the generate_from_source_pom.py script with the azure-storage-common self-reference, I'll submit a PR to fix this that'll need to be pulled into this branch.

@alzimmermsft Does that mean this solution will stop working and we should fall back to option 2 mentioned in PR description?

alzimmermsft · 2021-06-25T20:27:17Z

This PR should resolve the infinite recursion in the Python script: #22583

alzimmermsft · 2021-06-25T20:29:13Z

Appears you've found a bug in the generate_from_source_pom.py script with the azure-storage-common self-reference, I'll submit a PR to fix this that'll need to be pulled into this branch.

@alzimmermsft Does that mean this solution will stop working and we should fall back to option 2 mentioned in PR description?

The current approach is good, I'd rather wrangle a bit more infrastructure that only affects testing rather than move classes around before compilation and clean the up after it.

…e_pom.py

…zure#22583

rnpmsft · 2021-06-25T20:39:24Z

Appears you've found a bug in the generate_from_source_pom.py script with the azure-storage-common self-reference, I'll submit a PR to fix this that'll need to be pulled into this branch.

@alzimmermsft Does that mean this solution will stop working and we should fall back to option 2 mentioned in PR description?

The current approach is good, I'd rather wrangle a bit more infrastructure that only affects testing rather than move classes around before compilation and clean the up after it.

I think either way, the bug fix was necessary; both solutions rely on the META-INF being found in the test jar instead of living in the target folder for common, meaning the self-dependency would have to be there for both options.

I've mirrored your changes, let's see if CI is happy. I've pointed this PR to the feature/storage/DataMovement branch, but this PR addressed a more general issue; should I redirect it to the main branch to merge there when complete?

gapra-msft · 2021-06-25T20:41:19Z

Appears you've found a bug in the generate_from_source_pom.py script with the azure-storage-common self-reference, I'll submit a PR to fix this that'll need to be pulled into this branch.

@alzimmermsft Does that mean this solution will stop working and we should fall back to option 2 mentioned in PR description?

The current approach is good, I'd rather wrangle a bit more infrastructure that only affects testing rather than move classes around before compilation and clean the up after it.

I think either way, the bug fix was necessary; both solutions rely on the META-INF being found in the test jar instead of living in the target folder for common, meaning the self-dependency would have to be there for both options.

I've mirrored your changes, let's see if CI is happy. I've pointed this PR to the feature/storage/DataMovement branch, but this PR addressed a more general issue; should I redirect it to the main branch to merge there when complete?

Yes, I think that would be fine if you point this PR to main and then once this PR is merged, merge main into the DataMovement branch

…nt overriding parent POM + better relay execution goals, removed maven self-dependency (now using surefire classpath dependency)

…er changes to target path), fixed assembly paths for *nix-compatibility

…med variables/class names for clarity, final on new classes

rnpmsft · 2021-06-28T15:51:16Z

Changes until this point:

Moved getName() logic to TestNameProider used for the shared spec and extension
Removed accidental duplicate print code from testing
Changed the maven execution id's for better clarity, and to prevent overwriting parent POM's (current test shared source compile task was overwriting and causing common samples to not get compiled)
Removed maven <dependency> on self. This was causing a chicken and egg problem where from a clean build environment, the common package couldn't build as it couldn't find its tests jar... which doesn't build until it's test-compile phase. Instead, there's now an added classpath specified in surefire config, so build doesn't get blocked, but the dependency gets loaded at test time from local repository.
Tagged the Extension and NameProvider as final, is that correct?
Refactored TestHeaderExtension from Groovy to Java as the rest were already Java; do we want to keep extensions in Java?

rnpmsft · 2021-06-28T16:34:01Z

Changes until this point:

Moved getName() logic to TestNameProider used for the shared spec and extension

Removed accidental duplicate print code from testing

Changed the maven execution id's for better clarity, and to prevent overwriting parent POM's (current test shared source compile task was overwriting and causing common samples to not get compiled)

Removed maven <dependency> on self. This was causing a chicken and egg problem where from a clean build environment, the common package couldn't build as it couldn't find its tests jar... which doesn't build until it's test-compile phase. Instead, there's now an added classpath specified in surefire config, so build doesn't get blocked, but the dependency gets loaded at test time from local repository.

Tagged the Extension and NameProvider as final, is that correct?

Refactored TestHeaderExtension from Groovy to Java as the rest were already Java; do we want to keep extensions in Java?

@alzimmermsft Any issues here?

gapra-msft · 2021-06-28T16:41:13Z

Resolves #22458

alzimmermsft

Looks good to me!

gapra-msft and others added 3 commits June 16, 2021 13:16

Setup packages for the data movement library (Azure#22303)

75c2943

Removed blob data movement package, renamed common data movement to d… (

d61f4d4

Azure#22430)

Refactored test header logic into a global spock extension

a248605

rnpmsft requested review from amishra-dev, gapra-msft and kasobol-msft June 25, 2021 17:23

rnpmsft requested review from alzimmermsft, jaschrep-msft and rickle-msft as code owners June 25, 2021 17:23

ghost added the Storage Storage Service (Queues, Blobs, Files) label Jun 25, 2021

rnpmsft mentioned this pull request Jun 25, 2021

Test improvement: Get rid of duplicate setup code that prints test name, centralize that logic #22448

Closed

Added version tag for assembly dependency

7c99eb9

kasobol-msft reviewed Jun 25, 2021

View reviewed changes

Resolved infinite recursion on self-dependency in generate_from_sourc…

6963444

…e_pom.py

rnpmsft requested review from danieljurek, JimSuplizio, mitchdenny and weshaggard as code owners June 25, 2021 20:31

Changed infinite recursion fix to @alzimmermsft implementation from A…

6863ed1

…zure#22583

Rushi Patel added 3 commits June 25, 2021 17:18

Created TestNameProvider, decoupled TestHeaderExtension/StorageSpec

7d5f010

Removed duplicate header print, changed maven execution id's to preve…

f0a478f

…nt overriding parent POM + better relay execution goals, removed maven self-dependency (now using surefire classpath dependency)

Changed surefire dependency to local repository (resists edge case us…

5d7f382

…er changes to target path), fixed assembly paths for *nix-compatibility

Changed TestHeaderExtension from groovy to java for consistency, rena…

0f42c22

…med variables/class names for clarity, final on new classes

kasobol-msft approved these changes Jun 28, 2021

View reviewed changes

gapra-msft approved these changes Jun 28, 2021

View reviewed changes

alzimmermsft approved these changes Jun 28, 2021

View reviewed changes

kasobol-msft merged commit 7119e23 into Azure:main Jun 28, 2021

rnpmsft deleted the feature/storage/shared-test-header-logic branch June 28, 2021 19:22

kasobol-msft mentioned this pull request Aug 31, 2021

Log Uncaught and Unhandled Errors in Tests #23851

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactored test header logic into a global spock extension #22573

Refactored test header logic into a global spock extension #22573

rnpmsft commented Jun 25, 2021

rnpmsft commented Jun 25, 2021

kasobol-msft commented Jun 25, 2021

kasobol-msft Jun 25, 2021

kasobol-msft Jun 25, 2021

rnpmsft Jun 25, 2021

kasobol-msft Jun 25, 2021

rnpmsft Jun 25, 2021

alzimmermsft Jun 25, 2021

alzimmermsft commented Jun 25, 2021

kasobol-msft commented Jun 25, 2021

alzimmermsft commented Jun 25, 2021

alzimmermsft commented Jun 25, 2021

rnpmsft commented Jun 25, 2021

gapra-msft commented Jun 25, 2021

rnpmsft commented Jun 28, 2021 •

edited

Loading

rnpmsft commented Jun 28, 2021

gapra-msft commented Jun 28, 2021

alzimmermsft left a comment

Refactored test header logic into a global spock extension #22573

Refactored test header logic into a global spock extension #22573

Conversation

rnpmsft commented Jun 25, 2021

rnpmsft commented Jun 25, 2021

kasobol-msft commented Jun 25, 2021

kasobol-msft Jun 25, 2021

Choose a reason for hiding this comment

kasobol-msft Jun 25, 2021

Choose a reason for hiding this comment

rnpmsft Jun 25, 2021

Choose a reason for hiding this comment

kasobol-msft Jun 25, 2021

Choose a reason for hiding this comment

rnpmsft Jun 25, 2021

Choose a reason for hiding this comment

alzimmermsft Jun 25, 2021

Choose a reason for hiding this comment

alzimmermsft commented Jun 25, 2021

kasobol-msft commented Jun 25, 2021

alzimmermsft commented Jun 25, 2021

alzimmermsft commented Jun 25, 2021

rnpmsft commented Jun 25, 2021

gapra-msft commented Jun 25, 2021

rnpmsft commented Jun 28, 2021 • edited Loading

rnpmsft commented Jun 28, 2021

gapra-msft commented Jun 28, 2021

alzimmermsft left a comment

Choose a reason for hiding this comment

rnpmsft commented Jun 28, 2021 •

edited

Loading