Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Adding System.Drawing.Printing PrintDocument tests. #22868

Merged
merged 9 commits into from Aug 4, 2017

Conversation

KostaVlev
Copy link

Adding System.Drawing.Printing PrintDocument tests. (#20711)

Copy link
Contributor

@mellinoe mellinoe left a comment

Choose a reason for hiding this comment

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

Looks great overall. Thanks again for the contribution! I left one minor comment.

[InlineData("newDocument")]
public void DocumentName_SetValue_ReturnsExpected(string documentName)
{
if (documentName == null)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really understand what this is doing -- we already essentially test string.Empty with the second InlineData. We can probably just remove the null test case.

Copy link
Author

Choose a reason for hiding this comment

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

Oh I was thinking about something else when I wrote this lol. I probably should write the null case in separate test to assert that if document name is set to null it will return empty string ...

@mellinoe
Copy link
Contributor

mellinoe commented Aug 2, 2017

Also, many tests need to be disabled on Unix, and they all need to be made ConditionTheory/Fact's as usual.

{
public class PrintDocumentTests
{
private static bool AnyInstalledPrinters => PrinterSettings.InstalledPrinters.Count == 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry that I didn't mention this before. This same property is present in PageSettingsTests.cs. Can we move it to Helpers.cs and share it like Helpers.GdiplusIsAvailable?

Copy link
Author

Choose a reason for hiding this comment

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

Done!

@mellinoe
Copy link
Contributor

mellinoe commented Aug 4, 2017

Thanks for consolidating that helper. Looks good to me.

@mellinoe mellinoe merged commit f5feab3 into dotnet:master Aug 4, 2017
@karelz karelz modified the milestone: 2.1.0 Aug 14, 2017
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
)

* Adding System.Drawing.Printing PrintDocument tests. (dotnet/corefx#20711)

* Disabling some tests on Unix and making some Facts/Theories conditional.

* Fix tests conditional attributes.

* Fix tests conditional attributes.

* Adding missed test attributes.

* Fixing test attributes.

* Changing Assertion in AssertDefaultPageSettings() to expect PaperKind Letter. And skipping tests that fail on Unix.

* Updating PageSettings.PrintableArea assertions.

* Moving the check for  PrinterSettings.InstalledPrinters.Count == 0 to Helpers.cs and updating test attributes.


Commit migrated from dotnet/corefx@f5feab3
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
4 participants