Live tests by g2vinay · Pull Request #2812 · Azure/azure-sdk-for-java

g2vinay · 2018-12-20T23:11:45Z

Contains changes for Azure Key Vault Live Tests.

…enala as well).

…well)

azuresdkci · 2018-12-20T23:16:19Z

Can one of the admins verify this patch?

JonathanGiles · 2019-01-06T21:28:03Z

...est/java/com/microsoft/azure/keyvault/extensions/test/KeyVaultClientIntegrationTestBase.java


 	private static String getLiveVaultUri1() {
-		return getenvOrDefault("keyvault.vaulturi", "https://javasdktestvault.vault.azure.net");
+		return getenvOrDefault("KEYVAULT_VAULTURI", "https://javasdktestvault.vault.azure.net");


Out of curiosity - what motivated the changing of all key names to uppercase with underscores rather that periods?

Looks like periods are not processed correctly on linux system, and VSTS converts them to underscore, and converts environment variable names to uppercase anyway. Please see https://developercommunity.visualstudio.com/content/problem/328064/vsts-silently-converts-variable-names-to-uppercase.html for uppercase and https://stackoverflow.com/questions/50020314/escaping-environment-variable-names-in-vsts for underscores.

JonathanGiles · 2019-01-06T21:30:51Z

...ava/com/microsoft/azure/keyvault/extensions/test/KeyVaultKeyResolverDefaultProviderTest.java

         byte[] EK       = { (byte) 0x96, 0x77, (byte) 0x8B, 0x25, (byte) 0xAE, 0x6C, (byte) 0xA4, 0x35, (byte) 0xF9, 0x2B, 0x5B, (byte) 0x97, (byte) 0xC0, 0x50, (byte) 0xAE, (byte) 0xD2, 0x46, (byte) 0x8A, (byte) 0xB8, (byte) 0xA1, 0x7A, (byte) 0xD8, 0x4E, 0x5D };

-         String TEST_SECRET_NAME = SECRET_NAME + "1";
+         String TEST_SECRET_NAME = SECRET_NAME + "4";


It always makes me a little nervous when I see magic numbers being changed in unit tests. Why is this required?

Tests involve creating and deleting secrets.

Multiple Tests using same magic number for the secret name conflicts when a test is trying to create a secret and the previous Test requested a deletion. And the deletion is still in queue.
To avoid this conflict, changed the magic number for the test.

JonathanGiles · 2019-01-06T21:32:54Z

template/client/src/main/java/com/microsoft/azure/template/Hello.java

+     * Sample method.
+     * @return message
+     */
+    public final String getMessage() {
        return "hello";
    }
 }


This, and the package-info.java below, are unrelated changes, aren't they?

They both caused checkstyle and because of that the build fail.

weshaggard · 2019-01-08T00:21:13Z

.azure-pipelines/client.yml

@@ -2,32 +2,20 @@ trigger:
 - master

 jobs:


Looks like you have some merge conflicts to handle which should get this file updated to what is currently committed in the repo. As it stands you are reverting some changes that were committed because your branch is behind master.

Conflicts resolved

weshaggard · 2019-01-08T00:23:09Z

.azure-pipelines/client.test.live.yml

+   mvn test  -Dsurefire.rerunFailingTestsCount=3 $LOG_PARAMS -Dparallel=classes -DthreadCount=3 -DforkCount=1C -f pom.client.build.xml
+  displayName: 'Bash Script'
+  env:
+    ARM_CLIENTID: $(ARM_CLIENTID)


Are these 2 env variables the only difference between live vs not live?

There are more Variables defined in the variables tab of dev ops pipeline.

# Conflicts: # .azure-pipelines/client.test.yml # .azure-pipelines/client.yml # keyvault/data-plane/azure-keyvault/src/test/java/com/microsoft/azure/keyvault/test/AsyncOperationsTest.java # keyvault/data-plane/azure-keyvault/src/test/java/com/microsoft/azure/keyvault/test/SecretOperationsTest.java # keyvault/data-plane/pom.xml

weshaggard · 2019-01-16T23:04:19Z

.azure-pipelines/client.test.live.yml

+  - name: ARM_CLIENTKEY
+    value: $(java-keyvault-test-arm-client-key)
+
+resources:


Why do you need "resources: self"?

Not needed actually,
removed.

weshaggard · 2019-01-16T23:04:46Z

.azure-pipelines/client.test.live.yml

+- bash: |
+   LOG_PARAMS='-Dorg.slf4j.simpleLogger.defaultLogLevel=error -Dorg.slf4j.simpleLogger.log.org.apache.maven.cli.transfer.Slf4jMavenTransferListener=warn'
+   mvn test -Dhttp.keepAlive=false -Dsurefire.rerunFailingTestsCount=3 $LOG_PARAMS -f pom.client.build.xml 
+  displayName: 'Bash Script'


Please update this to represent what it is doing, like "Running Live Tests"

Updated it.

weshaggard · 2019-01-16T23:13:33Z

.azure-pipelines/client.test.live.yml

+queue:
+  name: Hosted VS2017
+steps:
+- bash: |


Is there a reason you aren't using the Maven@3 task to run the tests similar to https://github.com/Azure/azure-sdk-for-java/blob/master/.azure-pipelines/client.test.yml#L17?

In general we should try to do that to help be consistent and also that allows us to capture the test results into the test tab of the build which gives us a good view of the tests than ran and a history of them.

The environment variables were not read during the build when tried to configure as Maven task.

weshaggard · 2019-01-17T17:38:45Z

keyvault/data-plane/pom.xml

-<name>KEYVAULT_VAULTURI</name>
-       		<value>https://tifchen-keyvault-fancy.vault.azure.net</value>
+                          <name>KEYVAULT_VAULTURI</name>
+                          <value>https://azure-keyvault-3.vault.azure.net</value>


Looks like you are setting these in 3 places. One is here, one is below and one in the build definition. Do they need to be set in all these places?

Updated, the values should only be in environment variables section of pom.
Removed from other places.

weshaggard

The build pipeline looks good but please get @schaabs or @lenala to sign-off on the test changes.

lenala

Looks good to me.

…java into liveTests

weshaggard · 2019-01-17T21:10:33Z

keyvault/data-plane/pom.xml

@@ -325,8 +312,8 @@
            </includes>
            <environmentVariables>
              <test.mode>${testMode}</test.mode>
-              <KEYVAULT_VAULTURI>https://tifchen-keyvault-fancy.vault.azure.net</KEYVAULT_VAULTURI>
-              <KEYVAULT_VAULTURI_ALT>https://tifchen-keyvault-fancier.vault.azure.net</KEYVAULT_VAULTURI_ALT>
+              <KEYVAULT_VAULTURI>https://azure-keyvault-3.vault.azure.net</KEYVAULT_VAULTURI>


What about here? Should we also remove these?

I see you removed them from the YAML instead so there is no duplication.

headshot786 added 4 commits December 11, 2018 19:17

Adding Keyvault SDK to CI pipeline - (Includes changes from author: l…

d9901ca

…enala as well).

live tests for keyvault sdk (includes changes from author: lenala as …

083fef5

…well)

live test updates

6154137

Updated Test Recordings ran under new live test congiuration.

d204813

g2vinay requested review from lenala and weshaggard December 20, 2018 23:11

g2vinay requested review from JonathanGiles and mikeharder January 4, 2019 22:36

JonathanGiles reviewed Jan 6, 2019

View reviewed changes

weshaggard reviewed Jan 8, 2019

View reviewed changes

headshot786 and others added 3 commits January 9, 2019 17:22

Bringing back changes which got overwritten with master branch merge

f2655db

yml mvn command update.

585e5c2

g2vinay force-pushed the liveTests branch from 73c5ed3 to 585e5c2 Compare January 10, 2019 16:59

headshot786 and others added 4 commits January 10, 2019 13:25

Updated Test recordings

c3c4e88

Updated Secret Operations Test Test recordings

e226db2

updated yml with variable group

2682bd8

reverting changes in the files

efc825b

weshaggard reviewed Jan 16, 2019

View reviewed changes

headshot786 added 5 commits January 16, 2019 16:29

Updated Yml with Maven Task

e221646

Updating yml

9022fb9

adding variables as maven options

9faef73

changing environment variables as options

805ed55

Update yml with Bash script

fdaa2f2

weshaggard reviewed Jan 17, 2019

View reviewed changes

weshaggard approved these changes Jan 17, 2019

View reviewed changes

lenala approved these changes Jan 17, 2019

View reviewed changes

cleaninig maven properties

0232b36

schaabs approved these changes Jan 17, 2019

View reviewed changes

weshaggard and others added 2 commits January 17, 2019 10:43

Merge branch 'master' into liveTests

e22b3e5

Merge branch 'liveTests' of https://github.com/g2vinay/azure-sdk-for-…

addfe30

…java into liveTests

weshaggard reviewed Jan 17, 2019

View reviewed changes

weshaggard merged commit 02f0c7e into Azure:master Jan 17, 2019

JonathanGiles assigned g2vinay Feb 13, 2019

JonathanGiles added the Client This issue points to a problem in the data-plane of the library. label Feb 13, 2019

g2vinay deleted the liveTests branch July 13, 2023 01:54

Comments

Conversation

g2vinay commented Dec 20, 2018

Uh oh!

azuresdkci commented Dec 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lenala Jan 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

g2vinay Jan 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weshaggard left a comment

Choose a reason for hiding this comment

Uh oh!

lenala left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

lenala Jan 8, 2019 •

edited

Loading

g2vinay Jan 17, 2019 •

edited

Loading