Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/tag tools #7975

Merged
merged 19 commits into from Nov 4, 2020
Merged

Feature/tag tools #7975

merged 19 commits into from Nov 4, 2020

Conversation

lasote
Copy link
Contributor

@lasote lasote commented Oct 30, 2020

Changelog: Feature: Tagged tests and created a conftest.py to run the tests with pytest skipping the tests using not available tools (cmake, visual studio...).
Docs: omit

Some tests are tagged as tool_compiler because some way o another they suppose a settings.compiler will be detected. Most of them could be fixed but I didn't want to change tests in this PR.

  • Windows/Mac (pending to run the pytest test-suite in these OSS)
  • There is no tool version comparison yet, maybe we can implement it later
  • Tested with both unittest and functional docker images from Feature/docker images pytest conan_ci_jenkins#22
  • This PR shouldn't affect at all the current test-suite, but we could start to provide a new one more sophisticated based on tool availability.

NOTE: There is a SharedChainTest test failing (also in my Linux from several months ago) because something changed in the Linux oss regarding rpaths if I recall correctly. We should check.

Copy link
Member

@memsharded memsharded left a comment

Choose a reason for hiding this comment

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

This is looking very good. I totally agree that it is better to mark them and from there try to improve the tests, make tool-independent when possible (most times it will probably be just defining a base default profile not autodetected)

Copy link
Contributor

@jgsogo jgsogo left a comment

Choose a reason for hiding this comment

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

I need to know where we want to go with these changes, how the final process to launch the tests will look like. If tests are going to be inside different folders (unittest, functional, integration) then some of these marks are not needed, the test will fail if it isn't in the proper folder because tools won't be available.

conans/requirements_dev.txt Show resolved Hide resolved
conans/test/conftest.py Show resolved Hide resolved
conans/test/conftest.py Show resolved Hide resolved
Copy link
Contributor

@jgsogo jgsogo left a comment

Choose a reason for hiding this comment

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

Minor concern related to the conftest.py using the default naming. I prefer a different naming, or some explicit input to use the auto-detection and skip so many tests.

OTH, how will our CI force us to add those marks? Otherwise, it is very likely we will forget about them.

@memsharded
Copy link
Member

OTH, how will our CI force us to add those marks? Otherwise, it is very likely we will forget about them.

But if the mark is not added, the test will be run in the machines without the necessary tools and they will fail, isn't it the goal of this?

I think we don't want to use the folders as a mechanism to control tools at all. It is likely that most of the tests in "integration" will require some tool, but as the definitions of which tools have to be done anyway, then I don't see the need of using folders.

One of the issues that could happen is that some test is never run in our CI, maybe because it has some extreme conditions, lets say that a test explicitly requires "cmake 2.8.12" to test our "legacy" integration, but we don't have that in our CI (but the test has value for local development anyway?). Or there could be a typo. I see 2 possible approaches:

  • Have some tool in pytest that collects tests skipped, and report and the end of the whole build which tests haven't run
  • Force that every "mark" (with every version, if possible) has an entry defined in the config, even if it needs to be explicitly None to disable that test.

@memsharded memsharded changed the title `Feature/tag tools Feature/tag tools Nov 3, 2020
@lasote
Copy link
Contributor Author

lasote commented Nov 4, 2020

I need to know where we want to go with these changes, how the final process to launch the tests will look like. If tests are going to be inside different folders (unittest, functional, integration) then some of these marks are not needed, the test will fail if it isn't in the proper folder because tools won't be available.

Well, I think this PR enables the possibility of:

  • Launching "no tools" only tests (could be done first)
  • Launching only a concrete tool in several versions (e.g only cmake 3.0 and only cmake 3.9)
  • ...
  • And most important feature, keep everything as it is right now, so you can develop the new pipeline step by step.

The pipeline has to be defined and developed, this is just "tagging" tests.

Minor concern related to the conftest.py using the default naming. I prefer a different naming, or some explicit input to use the auto-detection and skip so many tests.

One of the requirements of the feature is to ease to run the tests for the community. I see reasonable that the default launcher run the tests for the available tool only.

OTH, how will our CI force us to add those marks? Otherwise, it is very likely we will forget about them.

It depends on the implementation of the pipeline, but if we control the test environment, and it is complete enough if a test is not correctly tagged, it will fail.

Force that every "mark" (with every version, if possible) has an entry defined in the config, even if it needs to be explicitly None to disable that test.

I think that is the right approach.

Copy link
Contributor

@jgsogo jgsogo left a comment

Choose a reason for hiding this comment

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

I was missing the purpose of this PR.

@memsharded memsharded added this to the 1.32 milestone Nov 4, 2020
@lasote lasote merged commit 598f83c into conan-io:develop Nov 4, 2020
redradist pushed a commit to redradist/conan that referenced this pull request Sep 12, 2021
* Tagged with pytest

* tox working

* py2 pytests version

* organized imports

* Test passing without tools

* Not modified tests

* Tests in parallel

* fixed test

* Some detection

* Fixed pkg_config detection

* tool_compiler

* fixed git

* fixed git

* Fix bad test

Co-authored-by: memsharded <james@conan.io>
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.

None yet

3 participants