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

[avocado_tests] added default_mapping test #3286

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dtzhou2
Copy link

@dtzhou2 dtzhou2 commented Jun 22, 2023

Added default mapping test and made it so all cleaner tests have the --no-update option as in issue #3254

Close #3254

Signed-off-by: Daniel Zhou [email protected]

Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines

  • [X ] Is the commit message split over multiple lines and hard-wrapped at 72 characters?
  • [ X] Is the subject and message clear and concise?
  • Does the subject start with [plugin_name] if submitting a plugin patch or a [section_name] if part of the core sosreport code?
  • [ X] Does the commit contain a Signed-off-by: First Lastname [email protected]?
  • [ X] Are any related Issues or existing PRs properly referenced via a Closes (Issue) or Resolved (PR) line?

@packit-as-a-service
Copy link

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

  • sudo yum install -y dnf-plugins-core on RHEL 8
  • sudo dnf install -y dnf-plugins-core on Fedora
  • dnf copr enable packit/sosreport-sos-3286
  • And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

Copy link
Member

@arif-ali arif-ali left a comment

Choose a reason for hiding this comment

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

Can you rebase to main branch please, which has the fix for the snap build

Also follow the guidelines and add the Signed-off-by line at the bottom, which can easily be done via git commit -s ...

Also add the Closes: line to the issue that it closes and resolves

def test_default_mapping(self):
self.assertFileExists('/etc/sos/cleaner/default_mapping')
self.assertOutputContains('Wrote mapping to')
with open('default_mapping') as ref:
Copy link
Contributor

Choose a reason for hiding this comment

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

What file you refer to here? When I run this test I get:

(1/8) tests/cleaner_tests/full_report/full_report_run.py:FullCleanTest.test_default_mapping: ERROR: [Errno 2] No such file or directory: 'default_mapping' (2.26 s)

Also I dont understand what you aim to compare here. self.files[0][1] should be /etc/sos/cleaner/default_mapping which will be populated (from scratch) by a mapping of sos report --clean - what content are you trying to compare with?

Maybe it makes sense to check if the file contains (some? all?) from hostname_map / ip_map / ipv6_map / mac_map / keyword_map / username_map strings (which should be in fact keys for nested maps)?

Copy link
Author

Choose a reason for hiding this comment

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

Yea that was what I was going for but I did not consider that default_mapping would generate machine specific information. Updating commit to change that

@dtzhou2 dtzhou2 force-pushed the dzhou-add-default-map-test branch from 16b068f to e63eb0d Compare June 26, 2023 15:44
Comment on lines 53 to 56
map_count = ref_data.count("map")
if map_count == 0:
assert(False)
assert True
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we simply:

self.assertNotEqual(ref_data.count("_map"), 0)

?

Also please squash this commit into the other one.

@pmoravec
Copy link
Contributor

pmoravec commented Jun 27, 2023

@TurboTurtle
Copy link
Member

This looks mostly correct - just echoing Pavel's comments above. Additionally please add a DCO line (you can do this with git commit -s or git commit -s --amend)

@dtzhou2 dtzhou2 force-pushed the dzhou-add-default-map-test branch from e63eb0d to f816b8f Compare July 3, 2023 14:05
@TurboTurtle
Copy link
Member

/packit retest-failed

@dtzhou2
Copy link
Author

dtzhou2 commented Jul 3, 2023

dont know whats broken here. Build just says there is a merge conflict

@TurboTurtle
Copy link
Member

There's a merge conflict with tests/cleaner_tests/basic_function_tests/report_with_mask.py - from 02d52f7 which already added --no-update here.

[avocado_tests] added default_mapping test

Added default mapping test and made it so all cleaner tests have the --no-update option as in issue sosreport#3254

Signed-off-by: Daniel Zhou <[email protected]>
@dtzhou2 dtzhou2 force-pushed the dzhou-add-default-map-test branch from f816b8f to 14995ba Compare July 3, 2023 17:42
self.assertOutputContains('Wrote mapping to')
with open('/etc/sos/cleaner/default_mapping') as ref:
ref_data = ref.read()
map_count = ref_data.count("map")
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this line redundant?

@pmoravec
Copy link
Contributor

ACK up to the one redundant line.

@arif-ali
Copy link
Member

@dtzhou2 are you still working on this PR?

@jcastill jcastill added the RedHat Eng Red Hat Engineering for review label Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changes Needed RedHat Eng Red Hat Engineering for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[tests] all cleaner tests to have --no-update option
5 participants