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.

Sign up for GitHub

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

Jump to bottom

Selftests user's default results directory cleanup #5557

Merged
merged 2 commits into from
Dec 23, 2022
Merged

Selftests user's default results directory cleanup #5557

merged 2 commits into from
Dec 23, 2022

Conversation

richtja
Copy link
Contributor

@richtja richtja commented Dec 19, 2022

This will update selftests to not store temporary results in user's
default results directory.

Reference: #5554
Signed-off-by: Jan Richter [email protected]

@richtja richtja added the bug label Dec 19, 2022
@richtja richtja added this to the #100 (the 100) milestone Dec 19, 2022
@richtja richtja self-assigned this Dec 19, 2022
@richtja richtja linked an issue Dec 19, 2022 that may be closed by this pull request
selftests/jobs/* leave test results behind #5554
Closed
@richtja richtja marked this pull request as draft December 19, 2022 16:53
@richtja richtja force-pushed the selftests_job_dir_clean branch 2 times, most recently from ef3688c to 343cc0b Compare December 20, 2022 10:49
@richtja richtja marked this pull request as ready for review December 20, 2022 12:03
clebergnu
clebergnu reviewed Dec 21, 2022
View reviewed changes
Copy link
Contributor

@clebergnu clebergnu left a comment

Choose a reason for hiding this comment

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

Hi @richtja,

This is an effective fix, but it has one drawback: it makes the example jobs (which are intended to be consumed by inexperienced users) much more complex than needed.

The only solution, without such a drawback, that I can think of, is to add support for the datadir.paths.logs_dir (aka --job-results-dir) to also consider an environment variable as its default value. This is not something we do to a lot of (or any) options in Avocado, but it's pretty standard in Linux/UNIX applications.

If we add a lookup to an AVOCADO_JOB_RESULTS_DIR environment variable, then we can add that only once to selftests/check.py and not have to tweak (and "pollute") any of the examples.

What do you think?

@richtja
Copy link
Contributor Author

richtja commented Dec 22, 2022 •
edited
Loading

Hi @richtja,

This is an effective fix, but it has one drawback: it makes the example jobs (which are intended to be consumed by inexperienced users) much more complex than needed.

The only solution, without such a drawback, that I can think of, is to add support for the datadir.paths.logs_dir (aka --job-results-dir) to also consider an environment variable as its default value. This is not something we do to a lot of (or any) options in Avocado, but it's pretty standard in Linux/UNIX applications.

If we add a lookup to an AVOCADO_JOB_RESULTS_DIR environment variable, then we can add that only once to selftests/check.py and not have to tweak (and "pollute") any of the examples.

What do you think?

Hi @clebergnu, I can see your point and agree that for an inexperienced user the examples might be confusing. But I don't like the idea that we will create an exception for datadir.paths.logs_dir in our config variables. IMO, if we want to do this, we should create support of environment variables for every config option. Maybe the other possible solution of this would be to remove job examples from selftests. And I would prefer this solution because IMO we should have separated examples and selftests from each other.

@clebergnu
Copy link
Contributor

Hi @richtja,
This is an effective fix, but it has one drawback: it makes the example jobs (which are intended to be consumed by inexperienced users) much more complex than needed.
The only solution, without such a drawback, that I can think of, is to add support for the datadir.paths.logs_dir (aka --job-results-dir) to also consider an environment variable as its default value. This is not something we do to a lot of (or any) options in Avocado, but it's pretty standard in Linux/UNIX applications.
If we add a lookup to an AVOCADO_JOB_RESULTS_DIR environment variable, then we can add that only once to selftests/check.py and not have to tweak (and "pollute") any of the examples.
What do you think?

Hi @clebergnu, I can see your point and agree that for an inexperienced user the examples might be confusing. But I don't like the idea that we will create an exception for datadir.paths.logs_dir in our config variables. IMO, if we want to do this, we should create support of environment variables for every config option. Maybe the other possible solution of this would be to remove job examples from selftests. And I would prefer this solution because IMO we should have separated examples and selftests from each other.

OK, I agree. Let's then:

  1. separate examples from tests (for now)
  2. evaluate/implement support for env variables in the avocado.core.settings code

@richtja richtja mentioned this pull request Dec 22, 2022
Open
@richtja
Copy link
Contributor Author

richtja commented Dec 22, 2022

Hi @richtja,
This is an effective fix, but it has one drawback: it makes the example jobs (which are intended to be consumed by inexperienced users) much more complex than needed.
The only solution, without such a drawback, that I can think of, is to add support for the datadir.paths.logs_dir (aka --job-results-dir) to also consider an environment variable as its default value. This is not something we do to a lot of (or any) options in Avocado, but it's pretty standard in Linux/UNIX applications.
If we add a lookup to an AVOCADO_JOB_RESULTS_DIR environment variable, then we can add that only once to selftests/check.py and not have to tweak (and "pollute") any of the examples.
What do you think?

Hi @clebergnu, I can see your point and agree that for an inexperienced user the examples might be confusing. But I don't like the idea that we will create an exception for datadir.paths.logs_dir in our config variables. IMO, if we want to do this, we should create support of environment variables for every config option. Maybe the other possible solution of this would be to remove job examples from selftests. And I would prefer this solution because IMO we should have separated examples and selftests from each other.

OK, I agree. Let's then:

1. separate examples from tests (for now)

2. evaluate/implement support for env variables in the `avocado.core.settings` code

So I separated examples from tests with force-push here, and I created issue #5568 to track env variables in the avocado.core.settings. Please have a look.

clebergnu
clebergnu requested changes Dec 22, 2022
View reviewed changes
Copy link
Contributor

@clebergnu clebergnu left a comment

Choose a reason for hiding this comment

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

Other than the preferable use of self.workdir for avocado.Test, this LGTM.

selftests/functional/serial/test_requirements.py Outdated Show resolved Hide resolved
selftests/functional/test_output.py Outdated Show resolved Hide resolved
@richtja
selftests leave test results behind fix
2186174 
This will update selftests to not store temporary results in user's
default results directory.

Reference: avocado-framework#5554
Signed-off-by: Jan Richter <[email protected]>
@richtja
selftests/check.py add check for cleanup result dir
3eb2673 
Let's ensure that the selftests/check.py doesn't leave test results
behind in user's default results directory.

Reference: avocado-framework#5554
Signed-off-by: Jan Richter <[email protected]>
@richtja
Copy link
Contributor Author

richtja commented Dec 22, 2022

Other than the preferable use of self.workdir for avocado.Test, this LGTM.

I did the fix with force-push here, please have a look.

clebergnu
clebergnu approved these changes Dec 23, 2022
View reviewed changes
Copy link
Contributor

@clebergnu clebergnu left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@clebergnu clebergnu merged commit ae596c0 into avocado-framework:master Dec 23, 2022
@richtja richtja deleted the selftests_job_dir_clean branch December 23, 2022 12:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clebergnu clebergnu clebergnu approved these changes

Assignees

@richtja richtja

Labels
bug
Projects
Default project
Archived in project
Milestone
#100 (the 100)
Development

Successfully merging this pull request may close these issues.

selftests/jobs/* leave test results behind
2 participants
@richtja @clebergnu