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

TMT enablement #83

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jkonecny12
Copy link
Member

@jkonecny12 jkonecny12 commented Aug 8, 2020

Enable TMT for Simpleline unit tests and smoke test.

This will be used by Packit and could be run locally.

TODO:

  • Enable local execution with prepare step which has artifacts. (Right now it can't import simpleline).

plans/smoke.fmf Outdated
@@ -0,0 +1,9 @@
summary: Run smoke test
discover:
Copy link

Choose a reason for hiding this comment

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

I believe discover steps does not need to be here. You are running a script in execute a script, thus discovered tests will be ignored I believe

Copy link

Choose a reason for hiding this comment

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

@psss right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for your review! Solved

@thrix
Copy link

thrix commented Aug 8, 2020

/packit test

@jkonecny12 jkonecny12 force-pushed the master-tmt-enablement branch 2 times, most recently from 96f2376 to 65fa2ad Compare August 8, 2020 15:58
@jkonecny12
Copy link
Member Author

/packit test

Copy link
Contributor

@poncovka poncovka left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@psss
Copy link

psss commented Aug 11, 2020

If you set the unit test path to the root of the git repo, import simpleline will work:

summary: Run unit tests
description: Run all unit tests for the Simpleline project.
contact: Jiri Konecny <[email protected]>
duration: 10m
path: /
test: ./tests/units/run_test.sh

However it still fails on other deps:

tmt run -vvv plan --name basic
...
ModuleNotFoundError: No module named 'gi'

Adding required python module into the plan will fix this:

prepare:
    how: install
    package: python3-gobject-base

@jkonecny12
Copy link
Member Author

If you set the unit test path to the root of the git repo, import simpleline will work:

summary: Run unit tests
description: Run all unit tests for the Simpleline project.
contact: Jiri Konecny <[email protected]>
duration: 10m
path: /
test: ./tests/units/run_test.sh

Yes, I know but in that case the Packit will not test installed source but instead it will test always the source code. I want to create new plan which will be ignored by Packit and that will work for the local run. I just didn't had time to finish that yet.

Thanks a lot for the prepare info. You saved my time in debugging that ;).

@jkonecny12
Copy link
Member Author

UPDATED:

  • Split plans to be make a correct preparation and test execution for local and pr-tests.

@jkonecny12
Copy link
Member Author

@psss What do you think, any space for improvement of my new configuration?

@jkonecny12
Copy link
Member Author

@thrix any idea about when my TMT configuration may work on cruncher?

Copy link
Contributor

@poncovka poncovka left a comment

Choose a reason for hiding this comment

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

I am sorry, but I don't understand it at all.

@psss
Copy link

psss commented Aug 12, 2020

Looks good. Perhaps, if you would prefer to have both tests defined in a single file, you can do it like this:

summary: Run unit tests
description: Run all unit tests for the Simpleline project.
contact: Jiri Konecny <[email protected]>
duration: 10m

/source:
    summary+: ' (source)'
    test: ./tests/units/run_test.sh
    path: /
    tag: local

/system:
    summary+: ' (system installed)'
    test: ./units/run_test.sh
    path: /tests
    tag: system

The extended summary is, of course, just an optional enhancement.

@thrix
Copy link

thrix commented Aug 16, 2020

@thrix any idea about when my TMT configuration may work on cruncher?

Wanted to make last week, did not work out :/ .. so this week, I plan to work on it from tomorrow

@jkonecny12
Copy link
Member Author

UPDATED:

  • Updated based on @psss suggestion. Thanks a lot!

@jkonecny12
Copy link
Member Author

jenkins, test this please

@TomasTomecek
Copy link

/packit build

1 similar comment
@jkonecny12
Copy link
Member Author

/packit build

@thrix
Copy link

thrix commented Aug 19, 2020

Ah F33 has branched, I am fixing fedora-32 now.

@thrix
Copy link

thrix commented Aug 19, 2020

/packit test

1 similar comment
@jkonecny12
Copy link
Member Author

/packit test

@thrix
Copy link

thrix commented Aug 19, 2020

Sorry I messed up, deploying another fix

@thrix
Copy link

thrix commented Aug 19, 2020

/packit test

@thrix
Copy link

thrix commented Aug 19, 2020

fixed :)

@sakalosj
Copy link

/packit test

@sakalosj
Copy link

/packit build

@jkonecny12
Copy link
Member Author

/packit test

@jkonecny12
Copy link
Member Author

/packit build

@jkonecny12
Copy link
Member Author

/packit build

Copy link
Contributor

@poncovka poncovka left a comment

Choose a reason for hiding this comment

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

Looks good to me now. Thank you!

@jkonecny12
Copy link
Member Author

jkonecny12 commented Oct 23, 2020

Seems this is still blocked by Cruncher:

 No 'script' defined in the execute step. 

I hope it will be fixed soon :(.

@thrix
Copy link

thrix commented Oct 26, 2020

@jkonecny12 we are almost there, but we got blocked on getting CentOS into AWS :/ Sorry for the delay

@VladimirSlavik
Copy link
Contributor

VladimirSlavik commented Oct 26, 2020

Do we really need to have all these files all over the repository? Isn't there some way to have them contained in .fmf, or tests/tmt, or such?

Also, it makes no sense to me that we're enabling TMT but all related filesystem bits are called FMF.

@psss
Copy link

psss commented Oct 26, 2020

Do we really need to have all these files all over the repository? Isn't there some way to have them contained in .fmf, or tests/tmt, or such?

The .fmf directory marks the root of the metadata tree (similar to what .git does) and it should be in the git repository root. As far are tests and plans are concerned, it is possible to choose arbitrary location for them. For example like this:

tests/plans ... for L2 metadata
tests/tests ... for L1 metadata

But storing test code & metadata under tests and plan configuration under plans seem to be a reasonable default.

Also, it makes no sense to me that we're enabling TMT but all related filesystem bits are called FMF.

The fmf format is used to store metadata with support for inheritance and elasticity (you can look at it as an extended yaml) while tmt is the test metadata specification and tool for easily running them from your laptop:

Hope this helps.

@jkonecny12
Copy link
Member Author

@jkonecny12 we are almost there, but we got blocked on getting CentOS into AWS :/ Sorry for the delay

Looking forward to that! Thanks for the update.

@VladimirSlavik
Copy link
Contributor

As far are tests and plans are concerned, it is possible to choose arbitrary location for them.

And that would happen in some configuration file? (Just asking to understand.)

But storing test code & metadata under tests and plan configuration under plans seem to be a reasonable default.

Well, both yes and no... I mean, from the testing point of view, certainly yes. But if I'm a wannabe contributor, open the repository and see the file listing, I see mostly stuff not really related to developing the thing. With some .hidden directories I can at least tell I can ignore them. Hope that explains why I'm asking.

Hope this helps.

Thank you, it does!

@jkonecny12
Copy link
Member Author

As far are tests and plans are concerned, it is possible to choose arbitrary location for them.

And that would happen in some configuration file? (Just asking to understand.)

But storing test code & metadata under tests and plan configuration under plans seem to be a reasonable default.

Well, both yes and no... I mean, from the testing point of view, certainly yes. But if I'm a wannabe contributor, open the repository and see the file listing, I see mostly stuff not really related to developing the thing. With some .hidden directories I can at least tell I can ignore them. Hope that explains why I'm asking.

Hope this helps.

Thank you, it does!

Hi @VladimirSlavik , please ask me directly. I think I'm able to answer most of your questions and it's much easier to discuss this on BJ/IRC than GH. ;)

discover+:
filter: tag:local
prepare:
- name: Install missing dependencies
Copy link
Contributor

Choose a reason for hiding this comment

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

This syntax is invalid. I copied that in my own playground, and filed a wrong bug as I was misled by the failure output.

prepare: is a dict, not a list. See the documentation

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks a lot for your insight. Interesting, I'm pretty sure that worked locally :D

Copy link
Member Author

@jkonecny12 jkonecny12 Nov 16, 2020

Choose a reason for hiding this comment

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

At the end this syntax seems to be supported, see:

https://github.com/psss/tmt/blob/master/plans/unit.fmf#L9

Seems that any stage could be specified like this to be able specify multiple different steps. However, it's not really required here.

Unfortunately, I can't find any sign about this in the documentation. Am I missing something @psss?

Copy link

Choose a reason for hiding this comment

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

Yes, steps support multiple definitions. This is documented in the multiple configs section and included in the examples as well. Were would you suggest to add an additional mention about this?

Copy link
Member Author

Choose a reason for hiding this comment

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

@psss
Ohh, thanks a lot for the pointers. I totally missed the multiple configs section. I guess the problem is that it's just in the Examples section and not mentioned in the Specification part. So maybe, it would be nice to add this to Metadata Specification section too with explanation that both variants are valid.

@martinpitt
Copy link
Contributor

FTR, I played around with that a bit, and https://github.com/martinpitt/python-simpleline/pull/1 now runs the tests correctly (except for a pylint version issue on older Fedoras, but that's not TMT's fault). Still requires some hacks, but these are said to go away with the TMT migration. So indeed let's wait with this for a bit until that happened. Until then, we have unit tests on github.

@jkonecny12
Copy link
Member Author

UPDATED:

@jkonecny12
Copy link
Member Author

/packit build

@jkonecny12
Copy link
Member Author

/packit build

@martinpitt martinpitt dismissed their stale review February 23, 2021 12:14

Removing my old nack -- LGTM if it works, thanks!

This configuration will work only for PR testing or gating.
PR-testing and Gating have to test unit-tests based on the installed
package, however, when we are running these tests in local then we want
to start the tests on source and not the installed package.

Create two plans to solve this issue.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants