Skip to content

qa/cephfs: simplify some code in test_multifs_auth.py #50665

Merged
rishabh-d-dave merged 1 commit intoceph:mainfrom
rishabh-d-dave:fs-qa-test-multifs-auth-simplify
Apr 5, 2023
Merged

qa/cephfs: simplify some code in test_multifs_auth.py #50665
rishabh-d-dave merged 1 commit intoceph:mainfrom
rishabh-d-dave:fs-qa-test-multifs-auth-simplify

Conversation

@rishabh-d-dave
Copy link
Contributor

@rishabh-d-dave rishabh-d-dave commented Mar 24, 2023 •
edited
Loading

Contribution Guidelines

Checklist

  • Tracker (select at least one)
    • References tracker ticket
    • Very recent bug; references commit where it was introduced
    • New feature (ticket optional)
    • Doc update (no ticket needed)
    • Code cleanup (no ticket needed)
  • Component impact
    • Affects Dashboard, opened tracker ticket
    • Affects Orchestrator, opened tracker ticket
    • No impact that needs to be tracked
  • Documentation (select at least one)
    • Updates relevant documentation
    • No doc update is appropriate
  • Tests (select at least one)
Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test dashboard cephadm
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox
  • jenkins test windows

@rishabh-d-dave rishabh-d-dave requested a review from a team March 24, 2023 18:55
@rishabh-d-dave rishabh-d-dave changed the title qa/cephfs: upgrade gen_mds_cap_str() in caps_helper qa/cephfs: simplify some code in test_multifs_auth.py Mar 24, 2023
@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented Mar 28, 2023 •
edited
Loading

It's easier to understand a test when the CephFS feature being tested is apparent at one go. In other words, a test case should be transparent. In general a test should look like this -

def test_some_feature(self):
    setup code for test

    enable the feature being tested
    test the feature

    teardown

test_multifs_auth.TestMDSCaps tests that auth caps assigned to a client for multiple FSs work fine. However the caps are not apparent when reading any of the test methods in TestMDSCaps because they are hidden under a layer of abstraction (i.e. under the call to `_create_client()``). This extra layer of abstraction makes test non-transparent.

The format of the test methods as a result is something like following -

def test_some_feature(self):
    setup code for test

    somemethod
    test the feature

    teardown

It would be nicer if we move to from latter to former example above.

Compare the code in main branch against the code on PR branch.

vshankar
vshankar reviewed Mar 29, 2023
View reviewed changes
@rishabh-d-dave rishabh-d-dave force-pushed the fs-qa-test-multifs-auth-simplify branch from 2d1f1c9 to 06a42bd Compare March 31, 2023 10:26
vshankar
vshankar reviewed Mar 31, 2023
View reviewed changes
@rishabh-d-dave rishabh-d-dave force-pushed the fs-qa-test-multifs-auth-simplify branch from 06a42bd to 2452d4e Compare March 31, 2023 12:26
vshankar
vshankar approved these changes Apr 4, 2023
View reviewed changes
Copy link
Contributor

@vshankar vshankar left a comment

Choose a reason for hiding this comment

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

LGTM

@rishabh-d-dave
qa/cephfs: simplify some code in test_multifs_auth.py
265abc7 
test_mutlifs_auth.TestMDSCaps._create_client() not only creates a client
but also generate caps strings for the client as per the parameter this
method receives and and then writes the keyring to all remote machines.
This creates confusion when reading code on test methods in TestMDSCaps.
Let's re-arrange this code such that this confusion is avoided.

Signed-off-by: Rishabh Dave <ridave@redhat.com>
@rishabh-d-dave rishabh-d-dave force-pushed the fs-qa-test-multifs-auth-simplify branch from 2452d4e to 265abc7 Compare April 5, 2023 10:52
@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented Apr 5, 2023

Caught a bug.

@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented Apr 5, 2023 •
edited
Loading

Tests passed - http://pulpito.front.sepia.ceph.com/rishabh-2023-04-05_10:58:42-fs:functional-wip-rishabh-fs-auth-subcmd-distro-default-smithi/

@rishabh-d-dave rishabh-d-dave mentioned this pull request Apr 5, 2023
Merged
4 tasks
@rishabh-d-dave rishabh-d-dave merged commit 53ac817 into ceph:main Apr 5, 2023
@rishabh-d-dave rishabh-d-dave deleted the fs-qa-test-multifs-auth-simplify branch April 5, 2023 14:18
This was referenced Dec 18, 2023
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lxbsz lxbsz lxbsz left review comments

@vshankar vshankar vshankar approved these changes

Assignees

No one assigned

Labels

cephfs Ceph File System cleanup tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@rishabh-d-dave @vshankar @lxbsz