Skip to content

Conversation

tromai
Copy link
Contributor

@tromai tromai commented Jun 27, 2024

Closes #779.

  • Convert the remaining test cases.
  • Move gradle jackson databind tests to the new test setup but make sure they are disabled for now.
  • Address the issue of running update command of the test utility script (chore: add exclude and include tag options to update command and add logging of each test case to update command #786).
  • Remove integration_tests.sh and integration _tests_docker.sh
  • Update dev docs about instructions on the tags of integration tests cases.
  • Handle the NO_NPM environment variable in Makefile.
  • Remove incorrect npm-registry tag for tests/integration/cases/gitlab_tinyMediaManager_purl/test.yaml
  • Go through the tests/e2e directory and clean up remaining files.

@tromai tromai self-assigned this Jun 27, 2024
@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Jun 27, 2024
@tromai tromai force-pushed the tromai/wrap-up-integration-tests-conversion branch from 6612358 to e39ea06 Compare July 3, 2024 06:23
@tromai
Copy link
Contributor Author

tromai commented Jul 3, 2024

Rebased to update the changes from #780

@tromai tromai force-pushed the tromai/wrap-up-integration-tests-conversion branch from 1ee517a to 52e790a Compare July 4, 2024 02:13
@tromai
Copy link
Contributor Author

tromai commented Jul 4, 2024

Rebased to update changes from #786

@tromai tromai marked this pull request as ready for review July 4, 2024 04:55
@tromai tromai requested a review from behnazh-w as a code owner July 4, 2024 04:55
@behnazh-w behnazh-w requested a review from benmss July 4, 2024 05:01
Copy link
Member

@behnazh-w behnazh-w left a comment

Choose a reason for hiding this comment

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

There are still configurations and expected_results under the tests/e2e directory, but I don't think they are used anywhere. Are these already converted?

@tromai
Copy link
Contributor Author

tromai commented Jul 10, 2024

@behnazh-w I believe all of them can be safely removed. They are still there because I missed them when converting the test cases. I will also go through the e2e directory and clean up the remaining files. I have addressed them in 73f432d

There are still configurations

  1. jackson_databind_config.yaml was originally used for the jackson databind integration tests
  • See the original usage -
    $RUN_MACARON analyze -c $WORKSPACE/tests/e2e/configurations/jackson_databind_config.yaml --skip-deps || log_fail
  • Within this commit from feat: map artifacts to commits via repo tags #508 - 0373bc0 - The jackson databind tests were changed from using an input yaml config to using a PURL for triggering the repo and commit finder. Therefore, this jackson_databind_config.yaml is not being used anywhere. I believe we can safely remove it.
  • As for the jackson databind tests, I have already converted the PURL version, hence I didn't notice this leftover input yaml config file.
  1. micronaut_test_config.yaml was originally used for the micronaut-test tests

expected_results

@behnazh-w
Copy link
Member

For the remaining compare_e2e_result.py under tests/e2e/, I think we can rename it to tests/analyze_json_output/compare_analyze_json_output.py? We can then use this comparison script and add a test case while addressing this issue.

@tromai
Copy link
Contributor Author

tromai commented Jul 11, 2024

For the remaining compare_e2e_result.py under tests/e2e/, I think we can rename it to tests/analyze_json_output/compare_analyze_json_output.py? We can then use this comparison script and add a test case while addressing this issue.

Yep I can definitely rename that script. Regarding issue #769 , do you prefer it to be addressed within this Pull Request too?

@behnazh-w
Copy link
Member

Yep I can definitely rename that script. Regarding issue #769 , do you prefer it to be addressed within this Pull Request too?

I am happy either way 🙂

@tromai
Copy link
Contributor Author

tromai commented Jul 11, 2024

Yep I can definitely rename that script. Regarding issue #769 , do you prefer it to be addressed within this Pull Request too?

I am happy either way 🙂

Sure, I will address #769 in a different Pull Request. Thanks!

@tromai
Copy link
Contributor Author

tromai commented Jul 11, 2024

@behnazh-w I have renamed the compare_e2e_result.py file in 1c65b9a

@behnazh-w
Copy link
Member

@tromai The Docker integration tests job has finished in 41s and as far as I can see, none of the tests have run. Can you please check?

@behnazh-w behnazh-w requested a review from benmss July 11, 2024 22:55
@tromai
Copy link
Contributor Author

tromai commented Jul 11, 2024

@tromai The Docker integration tests job has finished in 41s and as far as I can see, none of the tests have run. Can you please check?

Thanks for pointing it out. It was my mistake when setting up the tags. It should be resolved in 838ef86

@benmss
Copy link
Member

benmss commented Jul 12, 2024

I don't think I agree with the current implementation of the include tag list. To me it would make more sense to run all tests with any of the tags in the include list. If there really is a case where we want to run only the tests with all of a list of include tags, which implies the intersecting subset of one or more tag groups, then the solution is to give those tests a new tag and use that.

@tromai
Copy link
Contributor Author

tromai commented Jul 12, 2024

I don't think I agree with the current implementation of the include tag list. To me it would make more sense to run all tests with any of the tags in the include list.

@benmss
I agree that currently the set of tags we have can make the commands used in Makefile confusing. Therefore, I am suggesting this new setup of tags for all of our test cases:

  • A test case with python tag will be run with the Python package in our CI.
  • A test case with docker tag will be run with the Docker container in our CI.
  • A test case without any tag will not be run in the CI (But it would still be automatically picked up and run locally with python3 tests/integration/run.py run tests/integration/cases/...).
  • A test case with npm-registry tag will not run if NO_NPM is set to fail in our CI.

This new setup can still represent our use cases for subset of test cases while keeping the number of tag minimal:

  • Tests that are shared between Python package and Docker container can use docker and python.
  • Tests that are only run with Docker container will only contain docker.
  • Tests that are disabled will have no tag.

This set of tags would result in these commands for Makefile:

# Run the integration tests.
# Note: to disable npm tests set `NO_NPM` environment variable to `TRUE`.
.PHONY: integration-test
integration-test:
	if [ "${NO_NPM}" == "TRUE" ]; then \
    	echo "Note: NO_NPM environment variable is set to TRUE, so npm tests will be skipped."; \
		python ./tests/integration/run.py \
			run \
			--include-tag python \
			--exclude-tag npm-registry \
			./tests/integration/cases/...; \
	else \
		python ./tests/integration/run.py \
			run \
			--include-tag python \
			./tests/integration/cases/...; \
	fi

.PHONY: integration-test-docker
integration-test-docker:
	python ./tests/integration/run.py \
		run \
		--macaron scripts/release_scripts/run_macaron.sh \
		--include-tag docker \
		./tests/integration/cases/...

Am I understand the problem you mentioned correctly? 🤔

@benmss
Copy link
Member

benmss commented Jul 12, 2024

@tromai
I was advocating for changing the behaviour of the include list over something like that, but I suppose it's okay also.

I just meant that if you ran the tests with include-list=[tag1, tag2] then I'd expect all tests that have either tag1 or tag2 to run. Then for cases where you want only tests that have both tag1 and tag2, that is something worth being tagged with a new tag, e.g. tag1-tag2.

@tromai
Copy link
Contributor Author

tromai commented Jul 12, 2024

@tromai I was advocating for changing the behaviour of the include list over something like that, but I suppose it's okay also.

I just meant that if you ran the tests with include-list=[tag1, tag2] then I'd expect all tests that have either tag1 or tag2 to run. Then for cases where you want only tests that have both tag1 and tag2, that is something worth being tagged with a new tag, e.g. tag1-tag2.

Oh I see, yep I will change the behavior of --include-tag. @behnazh-w Please let me know if you are okay with this change too.

@behnazh-w
Copy link
Member

behnazh-w commented Jul 14, 2024

@tromai I was advocating for changing the behaviour of the include list over something like that, but I suppose it's okay also.
I just meant that if you ran the tests with include-list=[tag1, tag2] then I'd expect all tests that have either tag1 or tag2 to run. Then for cases where you want only tests that have both tag1 and tag2, that is something worth being tagged with a new tag, e.g. tag1-tag2.

Oh I see, yep I will change the behavior of --include-tag. @behnazh-w Please let me know if you are okay with this change too.

I agree with @benmss 's suggestion. I also suggest to improve the tag names:

  • macaron-python-package
  • macaron-docker-image
  • npm-registry-testcase

Trong Nhan Mai added 23 commits July 16, 2024 14:50
…clonedx maven deps

Signed-off-by: Trong Nhan Mai <trong.nhan.mai@oracle.com>
Signed-off-by: Trong Nhan Mai <trong.nhan.mai@oracle.com>
…late

Signed-off-by: Trong Nhan Mai <trong.nhan.mai@oracle.com>
…with deps resolution

Signed-off-by: Trong Nhan Mai <trong.nhan.mai@oracle.com>
Signed-off-by: Trong Nhan Mai <trong.nhan.mai@oracle.com>
…edx maven plugin

Signed-off-by: Trong Nhan Mai <trong.nhan.mai@oracle.com>
Signed-off-by: Trong Nhan Mai <trong.nhan.mai@oracle.com>
Signed-off-by: Trong Nhan Mai <trong.nhan.mai@oracle.com>
… script in Makefile

Signed-off-by: Trong Nhan Mai <trong.nhan.mai@oracle.com>
… tags of some test cases

Signed-off-by: Trong Nhan Mai <trong.nhan.mai@oracle.com>
… case

Signed-off-by: Trong Nhan Mai <trong.nhan.mai@oracle.com>
Signed-off-by: Trong Nhan Mai <trong.nhan.mai@oracle.com>
Signed-off-by: Trong Nhan Mai <trong.nhan.mai@oracle.com>
Signed-off-by: Trong Nhan Mai <trong.nhan.mai@oracle.com>
…ron.sh test case to use files within the test directory

Signed-off-by: Trong Nhan Mai <trong.nhan.mai@oracle.com>
Signed-off-by: Trong Nhan Mai <trong.nhan.mai@oracle.com>
Signed-off-by: Trong Nhan Mai <trong.nhan.mai@oracle.com>
Signed-off-by: Trong Nhan Mai <trong.nhan.mai@oracle.com>
…README on the tagging feature of run.py

Signed-off-by: Trong Nhan Mai <trong.nhan.mai@oracle.com>
Signed-off-by: Trong Nhan Mai <trong.nhan.mai@oracle.com>
Signed-off-by: Trong Nhan Mai <trong.nhan.mai@oracle.com>
Signed-off-by: Trong Nhan Mai <trong.nhan.mai@oracle.com>
Signed-off-by: Trong Nhan Mai <trong.nhan.mai@oracle.com>
@tromai tromai force-pushed the tromai/wrap-up-integration-tests-conversion branch from 9ee7e83 to a16877a Compare July 16, 2024 04:52
@tromai
Copy link
Contributor Author

tromai commented Jul 16, 2024

Rebased to obtain the changes in #795

@tromai tromai merged commit 429ba95 into staging Jul 17, 2024
@tromai tromai deleted the tromai/wrap-up-integration-tests-conversion branch July 17, 2024 01:11
art1f1c3R pushed a commit that referenced this pull request Nov 29, 2024
Signed-off-by: Trong Nhan Mai <trong.nhan.mai@oracle.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OCA Verified All contributors have signed the Oracle Contributor Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants