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

Add more testcases #1618

Closed
wants to merge 6 commits into from
Closed

Add more testcases #1618

wants to merge 6 commits into from

Conversation

dota17
Copy link
Contributor

@dota17 dota17 commented Aug 3, 2020

Major changes:

  • add test_family test case
  • rewrite test_track_order test case. I think the origin test case are not very straightly to show the track_order feature, based in that test case we cant't discover the bug referred in track_order not working on root group #1577.

@dota17 dota17 changed the title Temp Add more testcases Aug 3, 2020
@codecov
Copy link

codecov bot commented Aug 3, 2020

Codecov Report

Merging #1618 into master will increase coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1618      +/-   ##
==========================================
+ Coverage   88.47%   88.50%   +0.03%     
==========================================
  Files          17       17              
  Lines        2247     2254       +7     
==========================================
+ Hits         1988     1995       +7     
  Misses        259      259              
Impacted Files Coverage Δ
h5py/_hl/base.py 95.63% <0.00%> (ø)
h5py/_hl/dataset.py 93.15% <0.00%> (+0.03%) ⬆️
h5py/_hl/group.py 96.80% <0.00%> (+0.05%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a37eed8...ffba213. Read the comment docs.

Copy link
Member

@takluyver takluyver left a comment

Choose a reason for hiding this comment

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

Thanks. The test for the family driver will be a useful addition.

It makes sense to test track_order on the root group of a file, but I don't think we need to throw away the existing track_order tests to add that; they're pretty well written as they are.

docs/faq.rst Outdated Show resolved Hide resolved
docs/high/file.rst Outdated Show resolved Hide resolved
h5py/tests/test_file.py Outdated Show resolved Hide resolved
h5py/tests/test_file.py Outdated Show resolved Hide resolved
Comment on lines 399 to 401
grp = self.f.create_group('b', track_order=True)
self.f.create_group('a', track_order=True)
self.f.create_group('c', track_order=True)
Copy link
Member

Choose a reason for hiding this comment

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

Why did you scrap the .populate() method? The numeric vs alphabetical ordering is a neat way to test this (2 < 10 but '10' < '2'), and it was also checking that having a mixture of groups and datasets didn't interfere with the ordering.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

uhh, the populate() method is well designed, but not good for test_group, the existing bug have proved this, it is not a very straightforward way to show track_order feature for group, we should focus on testing root group and sub group, but not mixing group and dataset, so I propose to write it like this:

def populate(self, g, track_order):
    for i in range(9,0,-1):
        if i % 2 == 0:
            self.f.create_group(str(i), track_order)  # create root group
        else:
            g.create_group(str(i), track_order)       # create sub group

@ut.expectedFailure
def test_track_order(self):
    grp = self.f.create_group('0', track_order=True)   # --->True
    self.populate(grp, True)  # --->True
    expected = ['8', '6', '4', '2', '0']
    # this will fail until issue 1577 fixed
    self.assertEqual(list(self.f.keys(), expected))
   
    expected = ['9', '7', '5', '3', '1']
    self.assertEqual(list(grp.keys(), expected))

def test_no_track_order(self):
    grp = self.f.create_group('0', track_order=False)
    self.populate(grp, False)
    expected = ['0', '2', '4', '6', '8']
    self.assertEqual(list(self.f.keys(), expected))
   
    expected = ['1', '3', '5', '7', '9']
    self.assertEqual(list(grp.keys(), expected))   

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@takluyver Ready for your review.

h5py/tests/test_group.py Outdated Show resolved Hide resolved
h5py/tests/test_group.py Outdated Show resolved Hide resolved
h5py/tests/test_group.py Outdated Show resolved Hide resolved
@dota17 dota17 requested a review from takluyver August 19, 2020 03:48
@dota17 dota17 closed this Sep 10, 2020
@dota17 dota17 deleted the temp branch September 10, 2020 09:14
@dota17 dota17 restored the temp branch September 14, 2020 03:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants