Skip to content

Conversation

@rohan-dassani
Copy link
Contributor


This checklist is used to make sure that common guidelines for a pull request are followed.

Related command

General Guidelines

  • Have you run azdev style <YOUR_EXT> locally? (pip install azdev required)
  • Have you run python scripts/ci/test_index.py -q locally?

For new extensions:

About Extension Publish

There is a pipeline to automatically build, upload and publish extension wheels.
Once your pull request is merged into main branch, a new pull request will be created to update src/index.json automatically.
The precondition is to put your code inside this repository and upgrade the version in the pull request but do not modify src/index.json.

rohan-dassani and others added 30 commits August 26, 2022 13:16
	modified:   src/connectedk8s/azext_connectedk8s/_params.py
	modified:   src/connectedk8s/azext_connectedk8s/_utils.py
	modified:   src/connectedk8s/azext_connectedk8s/custom.py
	new file:   src/connectedk8s/azext_connectedk8s/patch-file.yaml
	modified:   src/connectedk8s/azext_connectedk8s/_utils.py
	modified:   src/connectedk8s/azext_connectedk8s/custom.py
	modified:   src/connectedk8s/azext_connectedk8s/custom.py
	modified:   src/connectedk8s/azext_connectedk8s/commands.py
	modified:   src/connectedk8s/azext_connectedk8s/custom.py
	new file:   src/connectedk8s/azext_connectedk8s/tests/latest/recordings/test_forcedelete.yaml
	modified:   src/connectedk8s/azext_connectedk8s/tests/latest/test_connectedk8s_scenario.py
	modified:   src/connectedk8s/azext_connectedk8s/custom.py
	renamed:    src/connectedk8s/azext_connectedk8s/patch-file.yaml -> src/connectedk8s/azext_connectedk8s/remove_crd_finalizer.yaml
	modified:   src/connectedk8s/azext_connectedk8s/tests/latest/recordings/test_forcedelete.yaml
	modified:   src/connectedk8s/azext_connectedk8s/tests/latest/test_connectedk8s_scenario.py
	modified:   src/connectedk8s/azext_connectedk8s/tests/latest/test_connectedk8s_scenario.py
	modified:   src/connectedk8s/azext_connectedk8s/custom.py
	modified:   src/connectedk8s/setup.py
	modified:   src/connectedk8s/azext_connectedk8s/tests/latest/recordings/test_forcedelete.yaml
	modified:   src/connectedk8s/azext_connectedk8s/tests/latest/recordings/test_forcedelete.yaml
	modified:   src/connectedk8s/azext_connectedk8s/tests/latest/recordings/test_forcedelete.yaml
	modified:   src/connectedk8s/setup.py
@ResourceGroupPreparer(name_prefix='conk8stest', location='eastus2euap', random_name_length=16)
def test_upgrade(self,resource_group):

managed_cluster_name = self.create_random_name(prefix='test-upgrade', length=24)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it mandatory to create the aks cluster and onboard it every time? Is there any way with which we can onboard once and run other commands/tests on top of it . If not all, atleast if we can reuse it for some tests so that we can save time for the overall test run

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AKS cluster creation everytime will take a lot of time. What is the avg time our current pipeline is taking?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all the tests actually run parallelly, the avg time is 50 mins but the point here is when the pipeline is triggered it just not only goes to our extension it sequentially goes to all of the extension spend around 2 mins each (again this is parallel too) and then executes our tests too. Pipeline ex - https://dev.azure.com/azclitools/internal/_build/results?buildId=33353&view=results

Having only one aks means , testing everything inside one function , this might actually increase the execution time of the tests and not sure if this is a cleaner approach

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why does it go to other extensions if we set the pipeline variable to run only our extension ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, do we know how many tests run in parallel at once? Even with parallel if it is taking 50 mins, we should get that clarity

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when we set the pipeline paramter , there is a parameter called user_parallelism = 8 by default , and I have seen 8 or maybe more extensions that are run parallely

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is number of extensions run parallely, I am asking about number of tests per extension parallely.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If tests per extension don't run parallely, then there would be no point in create aks and onboarding it everytime. Also, does the pipeline provide an option to run selected tests per extension or all tests will be run always

Copy link
Member

@wangzelin007 wangzelin007 Feb 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • The live test was not designed to be run every time. It will only run automatically at specific times, such as every Thursday morning.
  • Each extension will assign an agent to run the test, which is why you will see so many extensions running at the same time, but they all run for only two minutes, because they are skipped because the running conditions are not met.
  • If you want to run the tests on every commit, please remove the @live_only decorator and we will run the recording tests after every commit. (Please remember to upload the recording file generated by the live test)

@yonzhan
Copy link
Collaborator

yonzhan commented Feb 14, 2023

Connectedk8s

@rohan-dassani
Copy link
Contributor Author

@wangzelin007 can you please review this PR

class Connectedk8sScenarioTest(LiveScenarioTest):

def test_connectedk8s(self):
@live_only()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May I ask why these tests marked as @live_only()?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also have the same problem.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • If you want to run recording tests on every commit, please remove the @live_only decorator and we will run the recording tests after every commit. (Please remember to upload the recording file generated by the live test)
  • Otherwise none of the following tests will actually be executed:
    image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wangzelin007 if you remember we earlier attempted to try the recording mode but it didnt work out because of some issue in our connectedk8s connect code ( we are fetching tenant-id from cache because of which the tests were failing in recording mode) so for now we have decided to go with live mode only , we know this will not run as integration tests but till the time we find a solution for our command, we will make this a practice from our end that the member that raises the PR will trigger the CLI testing pipeline

@sirireddy12 if you have anything to add

@zhoxing-ms zhoxing-ms merged commit 59af7b9 into Azure:main Feb 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants