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

Set up Java logging with level TRACE in testsuite #3484

Merged
merged 1 commit into from
Mar 31, 2021

Conversation

agraul
Copy link
Member

@agraul agraul commented Mar 29, 2021

What does this PR change?

Add Java logging (TRACE) in the testsuite

GUI diff

No difference.

Before:

After:

  • DONE

Documentation

  • No documentation needed: just changing the testsuite settings

  • DONE

Test coverage

No code changes

  • DONE

Links

Needed for debugging https://github.com/SUSE/spacewalk/issues/14057

  • DONE

Changelogs

If you don't need a changelog check, please mark this checkbox:

  • No changelog needed

If you uncheck the checkbox after the PR is created, you will need to re-run changelog_test (see below)

Re-run a test

If you need to re-run a test, please mark the related checkbox, it will be unchecked automatically once it has re-run:

  • Re-run test "changelog_test"
  • Re-run test "backend_unittests_pgsql"
  • Re-run test "java_pgsql_tests"
  • Re-run test "schema_migration_test_oracle"
  • Re-run test "schema_migration_test_pgsql"
  • Re-run test "susemanager_unittests"
  • Re-run test "javascript_lint"
  • Re-run test "spacecmd_unittests"

@agraul agraul requested a review from Bischoff March 29, 2021 17:17
@agraul agraul force-pushed the java-testsuite-logging-trace branch 2 times, most recently from 051dcf4 to 5fef294 Compare March 29, 2021 17:49
Copy link
Contributor

@Bischoff Bischoff left a comment

Choose a reason for hiding this comment

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

see remarks - one of them whould have prevented the thing from working - please test

testsuite/features/step_definitions/command_steps.rb Outdated Show resolved Hide resolved
testsuite/features/core/first_settings.feature Outdated Show resolved Hide resolved
)
end

And(/^I restart the tomcat service$/) do
Copy link
Contributor

Choose a reason for hiding this comment

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

When

Copy link
Member Author

Choose a reason for hiding this comment

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

Tbh this is the most confusing part since I could find step implementations for both When and And, but I think they work the same. I'll change it to When, just wondered if you have a policy around that

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, it works the same. Yes there's a policvy. And is a shortcut for "same as previous line" in the feature test (in this case When), it has no reason to appear in the step definitions.

@srbarrios
Copy link
Member

srbarrios commented Mar 30, 2021

Personally, I would prefer a parameter in Sumaform that allows handling the log level.

We need to keep in mind that all additional Cucumber scenarios added in the core stage increase the time of testing results (in all kinds of test executions) also it will generate some extra entries in the HTML report.

Also, I think that can be useful for other kinds of users, to have this option in Sumaform.

@agraul agraul force-pushed the java-testsuite-logging-trace branch from f7c92bb to 4e643fd Compare March 30, 2021 09:30
@agraul
Copy link
Member Author

agraul commented Mar 30, 2021

Personally, I would prefer a parameter in Sumaform that allows handling the log level.

For me either way works. Generally non-qa sumaform users (read: devs) should be able to adjust logging as they go but having it inside sumaform is not something I'm against.

We need to keep in mind that all additional Cucumber scenarios added in the core stage increase the time of testing results (in all kinds of test executions) also it will generate some extra entries in the HTML report.

You're right that this increases the core stage run time. I don't know if a sumaform approach would be quicker and how often you run sumaform in relation to test suite executions (is there a sumaform execution before every core stage?).

The idea is not to leave this in for eternity, but rather to catch the heisenbug with additional logging and then decide if it makes sense to keep it or not. Again, if this should rather go into sumaform, we can move it there (now or after solving heisenbug)

@srbarrios
Copy link
Member

srbarrios commented Mar 30, 2021

Personally, I would prefer a parameter in Sumaform that allows handling the log level.

For me either way works. Generally non-qa sumaform users (read: devs) should be able to adjust logging as they go but having it inside sumaform is not something I'm against.

We need to keep in mind that all additional Cucumber scenarios added in the core stage increase the time of testing results (in all kinds of test executions) also it will generate some extra entries in the HTML report.

You're right that this increases the core stage run time. I don't know if a sumaform approach would be quicker and how often you run sumaform in relation to test suite executions (is there a sumaform execution before every core stage?).

The idea is not to leave this in for eternity, but rather to catch the heisenbug with additional logging and then decide if it makes sense to keep it or not. Again, if this should rather go into sumaform, we can move it there (now or after solving heisenbug)

Ah ok, I did not catch that this was temporary to find an issue.
Ok, so let's keep it here, for now, just to avoid re-doing it in sumaform, but it might be interesting to create a card in sumaform, as that could be a "nice to have" there some day.

@agraul agraul merged commit 7c3089b into uyuni-project:master Mar 31, 2021
@agraul
Copy link
Member Author

agraul commented Mar 31, 2021

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.

3 participants