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

Fix DefaultColumnWidth #1181

Merged
merged 7 commits into from
Oct 11, 2023
Merged

Conversation

Bykiev
Copy link
Collaborator

@Bykiev Bykiev commented Sep 8, 2023

This PR fixes DefaultColumnWidth type and implementation. Due to POI bug it returned baseColumnWidth, which is integer. This is a breaking change.

Fixes #943

This PR fixes DefaultColumnWidth type and implementation. Due to POI bug it returned baseColumnWidth, which is integer. This is a breaking change.

Fixes nissl-lab#943
@Bykiev Bykiev marked this pull request as draft September 8, 2023 15:28
@Bykiev
Copy link
Collaborator Author

Bykiev commented Sep 8, 2023

This seems to be incorrect: when retrieving column width we should check first if the column width is set, otherwise if defaultColWidthspecified, if not, we should check baseColWidth or fallback to 8.43 otherwise

@Bykiev
Copy link
Collaborator Author

Bykiev commented Sep 9, 2023

Failing test doesn't related to changes, seems #1132 has the same issue with parallel test execution.
@lahma, @tonyqus, do you have any ideas how to fix it or maybe how to run IO tests sync?

@lahma
Copy link
Collaborator

lahma commented Sep 10, 2023

Would it just make more sense in the test case (TestOpenFileThenSaveDelete) to check that neither of the assigned test files exist? Honestly I don't fully understand the test case.

Assert.False(File.Exists(tempFile));
Assert.False(File.Exists(tempFile2));

@Bykiev
Copy link
Collaborator Author

Bykiev commented Sep 11, 2023

Would it just make more sense in the test case (TestOpenFileThenSaveDelete) to check that neither of the assigned test files exist? Honestly I don't fully understand the test case.

Assert.False(File.Exists(tempFile));
Assert.False(File.Exists(tempFile2));

Yes, this test can be rewritten, but then another IO-dependent test can fail. We need to resolve it globally

@lahma
Copy link
Collaborator

lahma commented Sep 11, 2023

I wouldn't trust the RunSerialyAndSweepTmpFilesAttribute at all, it's implementation looks quite fishy and expects that process-wide locks would work for test runners running multiple assemblies concurrently (they don't). NUnit does not run tests concurrently by default so there's that too.

I'd say removing the attribute and adjusting the test case to check for specific items would be the way to go. Some cleanup logic could go somewhere like assembly fixture teardown.

@Bykiev Bykiev marked this pull request as ready for review September 14, 2023 18:50
@tonyqus tonyqus added this to the NPOI 2.7.0 milestone Oct 11, 2023
@tonyqus
Copy link
Member

tonyqus commented Oct 11, 2023

LGTM

@tonyqus tonyqus merged commit dbd4020 into nissl-lab:master Oct 11, 2023
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

excel col default width problem,
3 participants