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

test PRC's consistency with iRODS server in mapping GenQuery1 column names to numeric keys #647

Merged
merged 4 commits into from
Nov 12, 2024
Merged

test PRC's consistency with iRODS server in mapping GenQuery1 column names to numeric keys #647

merged 4 commits into from
Nov 12, 2024

Conversation

d-w-moore
Copy link
Collaborator

No description provided.

@d-w-moore d-w-moore force-pushed the db_model_columns_test_642_643 branch from a99b513 to 53ffe24 Compare October 22, 2024 12:49
@d-w-moore
Copy link
Collaborator Author

d-w-moore commented Oct 22, 2024 •
edited
Loading

What still remains is to diff the full set against rodsGenQuery.h in a unit test

@d-w-moore d-w-moore marked this pull request as draft October 22, 2024 12:52
@trel
Copy link
Member

trel commented Oct 22, 2024

i see a replacement of six in here that ... should have been in the other PR?

@d-w-moore
Copy link
Collaborator Author

i see a replacement of six in here that ... should have been in the other PR?

Try reloading.

@trel
Copy link
Member

trel commented Oct 22, 2024

diff is against main, so I suppose that's why it's showing here. If the other PR is merged first, this diff will show differently.

Carry on.

@d-w-moore
Copy link
Collaborator Author

d-w-moore commented Oct 22, 2024 •
edited
Loading

diff is against main, so I suppose that's why it's showing here. If the other PR is merged first, this diff will show differently.

Carry on.

You're right though, there is a six dependency in there. Good catch @trel . Missed that....

@d-w-moore d-w-moore force-pushed the db_model_columns_test_642_643 branch from 18674cb to 4906dbf Compare October 22, 2024 13:13
@trel
Copy link
Member

trel commented Oct 22, 2024

let's get the six removals done in #640 - keep them all together.

trel
trel reviewed Oct 30, 2024
View reviewed changes
irods/test/meta_test.py Outdated Show resolved Hide resolved
@d-w-moore d-w-moore force-pushed the db_model_columns_test_642_643 branch from b081754 to 143ad0b Compare October 31, 2024 10:34
trel
trel reviewed Oct 31, 2024
View reviewed changes
irods/models.py Outdated Show resolved Hide resolved
@d-w-moore d-w-moore self-assigned this Nov 1, 2024
@d-w-moore d-w-moore marked this pull request as ready for review November 1, 2024 07:39
@d-w-moore
Copy link
Collaborator Author

d-w-moore commented Nov 1, 2024 •
edited
Loading

Running the 643 test on a reversion of the column corrections produces this, which is ... good.

$ python -m unittest irods.test.meta_test.TestMeta.test_that_all_column_mappings_are_uniquely_and_properly_defined__issue_643
F
======================================================================
FAIL: test_that_all_column_mappings_are_uniquely_and_properly_defined__issue_643 (irods.test.meta_test.TestMeta)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/daniel/ppy3/python-irodsclient/irods/test/meta_test.py", line 626, in test_that_all_column_mappings_are_uniquely_and_properly_defined__issue_643
    self.assertEqual( sr, allowed_outliers )
AssertionError: Items in the first set but not the second:
'COL_META_COLL_ATTR_UNITS'
'COL_TICKET_ALLOWED_GROUP'
'COL_TICKET_ALLOWED_USER'
'COL_META_RESC_ATTR_UNITS'

----------------------------------------------------------------------
Ran 1 test in 0.135s

FAILED (failures=1)

@d-w-moore d-w-moore changed the title test PRC's consistency with iRODS server's GenQuery1 column "names" and numeric keys test PRC's consistency with iRODS server in mapping GenQuery1 column names to numeric keys Nov 1, 2024
korydraughn
korydraughn reviewed Nov 1, 2024
View reviewed changes
Copy link
Contributor

@korydraughn korydraughn left a comment

Choose a reason for hiding this comment

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

Looking good.

What's left for this PR? Are we almost ready for merging?

irods/test/meta_test.py Outdated Show resolved Hide resolved
irods/models.py Show resolved Hide resolved
alanking
alanking reviewed Nov 1, 2024
View reviewed changes
Copy link
Contributor

@alanking alanking left a comment

Choose a reason for hiding this comment

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

Overall seems fine. Had some questions and minor suggestions for improvement.

irods/models.py Outdated Show resolved Hide resolved
irods/test/meta_test.py Show resolved Hide resolved
irods/models.py Show resolved Hide resolved
@d-w-moore
Copy link
Collaborator Author

Looking good.

What's left for this PR? Are we almost ready for merging?

Yes I think we're pretty much there, upon comments resolution.

@d-w-moore
Copy link
Collaborator Author

Looking good.

What's left for this PR? Are we almost ready for merging?

I think it is ready, yes. Need a squash.

@d-w-moore d-w-moore force-pushed the db_model_columns_test_642_643 branch from e0dbb64 to 9c8ccdb Compare November 2, 2024 12:53
@d-w-moore
Copy link
Collaborator Author

This has now had some reasonable squashing done.

alanking
alanking approved these changes Nov 4, 2024
View reviewed changes
Copy link
Contributor

@alanking alanking left a comment

Choose a reason for hiding this comment

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

Nice. Please await one more approval before #'ing.

korydraughn
korydraughn reviewed Nov 4, 2024
View reviewed changes
Copy link
Contributor

@korydraughn korydraughn left a comment

Choose a reason for hiding this comment

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

Changes look good.

The body of one of the commits is wrapped in parentheses. Is that intentional?

@trel
Copy link
Member

trel commented Nov 4, 2024

parentheses don't add anything in my opinion.

@d-w-moore
Copy link
Collaborator Author

parentheses don't add anything in my opinion.

Will look&correct

@d-w-moore d-w-moore force-pushed the db_model_columns_test_642_643 branch from 9c8ccdb to 16d2bf1 Compare November 4, 2024 15:59
@d-w-moore
Copy link
Collaborator Author

Done, no paren-veloping. Assuming there is approval, do we want more squashing?

@trel
Copy link
Member

trel commented Nov 4, 2024

i vote for them being separate (as is) - easier to follow the fix and the tests.

korydraughn
korydraughn approved these changes Nov 4, 2024
View reviewed changes
Copy link
Contributor

@korydraughn korydraughn left a comment

Choose a reason for hiding this comment

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

I agree. Keeping things as-is is good.

Pound it.

@d-w-moore
[irods#642,irods#643] build a complete list of GenQuery1 column keys …
076a251 
…and names
@d-w-moore
[irods#642] correct column mapping definitions known to be faulty.
9484711
@d-w-moore
[irods#643] Test for mismatched column mapping definitions in ModelBase.
280c37b 
This test checks against a header file in the iRODS
development package, and therefore is skipped unless that
package is pre-installed.
@d-w-moore
[irods#642] test column names against values in a typical query result.
f5946cd 
The test should succeed only if no incorrect name-to-number column mappings
are loaded for COL_META_COLL_ATTR_UNITS.  When present, these faulty
mappings can create redundant and/or incorrect column-to-value entries in a
CollectionMeta query result.
@d-w-moore
Copy link
Collaborator Author

I agree. Keeping things as-is is good.

Pound it.

Done!

@alanking alanking merged commit 5ae9590 into irods:main Nov 12, 2024
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trel trel trel left review comments

@alanking alanking alanking approved these changes

@korydraughn korydraughn korydraughn approved these changes

Assignees

@d-w-moore d-w-moore

Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@d-w-moore @trel @korydraughn @alanking