Skip to content

Conversation

lsgordon
Copy link
Contributor

  • Tests added and passed if fixing a bug or adding a new feature

  • ✅ All code checks passed.
    Added tests to the ExcelFormatter class, which gets the file up to 93% test coverage in total. The only exception is the .write function, (otherwise writing to excel in the first place is not tested) but is already most definitely covered, but should be tested further.

  • Leo

@simonjayhawkins simonjayhawkins added Testing pandas testing functions or related to the test suite IO Excel read_excel, to_excel labels Jun 30, 2025
Comment on lines +488 to +492
breaking_row_count = 2**20 + 1
breaking_col_count = 2**14 + 1

# Test for too many rows
row_df = DataFrame(np.zeros(shape=(breaking_row_count, 1)))
Copy link
Member

Choose a reason for hiding this comment

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

This there a way to test this without actually allocating such a large array

Copy link
Member

Choose a reason for hiding this comment

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

Otherwise, I'd prefer not to include this test this case due to the large memory allocation

df = DataFrame({"A": [1, 2], "B": [3, 4]})
msg = "Not all names specified in 'columns' are found"
with pytest.raises(KeyError, match=msg):
ExcelFormatter(df, cols=columns)
Copy link
Member

Choose a reason for hiding this comment

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

Generally, could you write tests that exercise to_excel instead of ExcelFormatter and its private methods instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes a lot of sense.

@lsgordon lsgordon closed this Jul 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

IO Excel read_excel, to_excel Testing pandas testing functions or related to the test suite

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants