◐ Shell
reader mode source ↗
Skip to content

bpo-34482: Add tests for proper handling of non-UTF-8-encodable strin…#8878

Merged
taleinat merged 7 commits into
python:masterfrom
izbyshev:bpo-34482
Oct 23, 2018
Merged

bpo-34482: Add tests for proper handling of non-UTF-8-encodable strin…#8878
taleinat merged 7 commits into
python:masterfrom
izbyshev:bpo-34482

Conversation

@izbyshev

@izbyshev izbyshev commented Aug 23, 2018 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

bpo-34482: Add tests for proper handling of non-UTF-8-encodable strin… …
97037bd 
…gs in datetime classes

A follow-up of bpo-34454.
@izbyshev izbyshev mentioned this pull request Aug 23, 2018
Closed
@pganssle

pganssle commented Aug 23, 2018

Copy link
Copy Markdown
Member

II think rather than doing try/except, it's best use expectedFailure. Unfortunately, it doesn't seem like there is a way to do conditional expected failures like there is with pytest.xfail.

Alternatively, maybe use skipIf?

@pganssle

pganssle commented Aug 23, 2018

Copy link
Copy Markdown
Member

I think this also needs tests for date.strftime as well, no?

@pganssle

pganssle commented Aug 23, 2018 •
edited
Loading

Copy link
Copy Markdown
Member

I made a PR against @izbyshev's branch that fixes this bug as part of adding the test suite.

I can move that to a separate PR against master if preferred.

@taleinat

taleinat commented Aug 23, 2018

Copy link
Copy Markdown
Contributor

IMO a NEWS entry isn't needed here, since this only affects tests.

@taleinat taleinat added the tests Tests in the Lib/test dir label Aug 23, 2018
@pganssle

pganssle commented Aug 23, 2018

Copy link
Copy Markdown
Member

By the way, in light of the fact that I have a PR to fix this, I withdraw my comments about expectedFailure and such. The whole point of expectedFailure is to make it clear that you are testing known-pathological behavior so it's not misinterpreted (and so that you can check in an automated way whether your expectations are false).

Since it will be fixed almost immediately it's a pointless bit of formalism to explicitly mark them as failing just to have them switched over to succeeding immediately.

@izbyshev

izbyshev commented Aug 23, 2018

Copy link
Copy Markdown
Contributor Author

@pganssle

I think this also needs tests for date.strftime as well, no?

My tests are in TestDate and TestTime test cases. TestDateTime is a subclass of TestDate, so my tests run for all three classes.

@pganssle

pganssle commented Aug 23, 2018

Copy link
Copy Markdown
Member

@taleinat Per discussion on izbyshev#1, I think it might be good to just do these as two separate PRs, if this one is ready to merge. It would be inconvenient for me to update my PR in response to review comments if it has to go through PRs to Alexey's fork first, and it's probably better to centralize the review comments on the main fork.

@izbyshev

izbyshev commented Aug 23, 2018

Copy link
Copy Markdown
Contributor Author

Thanks for the review, @pganssle and @taleinat! I've updated the PR. I haven't removed try/except since @pganssle wants to merge his PR on top of mine.

@izbyshev

izbyshev commented Aug 23, 2018

Copy link
Copy Markdown
Contributor Author

@pganssle And I've eventually decided to add a separate test for datetime.strftime since it won't hurt :)

Remove check for strftime() return value …
51bd826 
Its behavior across platforms is inconsistent and hard to test.
@izbyshev

izbyshev commented Aug 24, 2018

Copy link
Copy Markdown
Contributor Author

I've removed assertEqual from strftime tests per discussion with @pganssle.

@pganssle pganssle mentioned this pull request Aug 28, 2018
Closed
@izbyshev

izbyshev commented Aug 29, 2018

Copy link
Copy Markdown
Contributor Author

@taleinat Would you give this PR another look?

@vstinner

vstinner commented Oct 22, 2018

Copy link
Copy Markdown
Member

I just merged the PR #8959: you may have to update/rebase this PR (that I didn't review it).

@izbyshev

izbyshev commented Oct 22, 2018

Copy link
Copy Markdown
Contributor Author

Thanks, @vstinner, but this PR doesn't need rebasing right now.

BTW, I don't know why Travis is unhappy. Could anybody tell it to try again?

@taleinat

taleinat commented Oct 22, 2018

Copy link
Copy Markdown
Contributor

BTW, I don't know why Travis is unhappy. Could anybody tell it to try again?

Restarted.

Two runs, one of the a "docs" run, ran out of memory. I hope it's just transient.

@taleinat

taleinat commented Oct 22, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

BTW, I don't know why Travis is unhappy. Could anybody tell it to try again?

Restarted.

Two runs, one of the a "docs" run, ran out of memory. I hope it's just transient.

The test run succeeded. The "docs" run fails due to recent changes having left it a broken state in master for a short while; this just needs a merge/rebase. I'm on it.

24 hidden items Load more…
taleinat
taleinat approved these changes Oct 22, 2018
View reviewed changes

@taleinat taleinat left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hide comment

LGTM

vstinner
vstinner approved these changes Oct 22, 2018
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

LGTM. I love to see more tests on edge cases!

@izbyshev

izbyshev commented Oct 22, 2018

Copy link
Copy Markdown
Contributor Author

Thanks to @taleinat for helping with Travis and reviewing, and to @vstinner for reviewing!

@taleinat

taleinat commented Oct 23, 2018

Copy link
Copy Markdown
Contributor

I'm backporting this to 3.7 similarly to the related PRs GH-8862 and GH-8959, for consistency and easier future backporting.

@taleinat taleinat merged commit 3b0047d into python:master Oct 23, 2018
@miss-islington

miss-islington commented Oct 23, 2018

Copy link
Copy Markdown
Contributor

Thanks @izbyshev for the PR, and @taleinat for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

@miss-islington

miss-islington commented Oct 23, 2018

Copy link
Copy Markdown
Contributor

Thanks @izbyshev for the PR, and @taleinat for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

@bedevere-bot

bedevere-bot commented Oct 23, 2018

Copy link
Copy Markdown

GH-10049 is a backport of this pull request to the 3.7 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 23, 2018
@miss-islington
bpo-34482: test datetime classes' handling of non-UTF-8-encodable str… …
5153433 
…ings (pythonGH-8878)

(cherry picked from commit 3b0047d)

Co-authored-by: Alexey Izbyshev <izbyshev@ispras.ru>
miss-islington added a commit that referenced this pull request Oct 23, 2018
@miss-islington
bpo-34482: test datetime classes' handling of non-UTF-8-encodable str… …
313e501 
…ings (GH-8878)

(cherry picked from commit 3b0047d)

Co-authored-by: Alexey Izbyshev <izbyshev@ispras.ru>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pganssle pganssle pganssle left review comments

@vstinner vstinner vstinner approved these changes

@taleinat taleinat taleinat approved these changes

Assignees

No one assigned

Labels

skip news tests Tests in the Lib/test dir

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@izbyshev @pganssle @taleinat @vstinner @miss-islington @bedevere-bot @the-knights-who-say-ni