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

Typo: delete duplicated test case #12662

Merged
merged 2 commits into from Apr 4, 2019
Merged

Typo: delete duplicated test case #12662

merged 2 commits into from Apr 4, 2019

Conversation

MakDon
Copy link
Contributor

@MakDon MakDon commented Apr 2, 2019

Delete a duplicated test case in re_tests.
In the original version, line 109 seems to be the same with 112, which means line 112 is duplicated.
According to "Trivial changes, like fixing a typo, do not need an issue.", no issue was created in the https://bugs.python.org.

@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

@nanjekyejoannah
Copy link
Member

nanjekyejoannah commented Apr 2, 2019

@MakDon There is no need then to put "bpo-None:" Just write the PR title in this case.

see : #12613

@MakDon MakDon changed the title bpo-None:Typo-delete duplicated test case Typo: delete duplicated test case Apr 3, 2019
@MakDon
Copy link
Contributor Author

MakDon commented Apr 3, 2019

@nanjekyejoannah Thanks.Title fixed.

Copy link
Member

@tirkarthi tirkarthi left a comment

Choose a reason for hiding this comment

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

LGTM. commit where tests were changed : bd48d27 .

cc: @serhiy-storchaka

@serhiy-storchaka
Copy link
Member

Comparing with other tests, it is more likely this test should contain \r instead of \n.

edit line 112 .
Before:
('(?s)a.b', 'a\nb', SUCCEED, 'found', 'a\nb'),
After:
('(?s)a.b', 'a\rb', SUCCEED, 'found', 'a\rb'),
@MakDon
Copy link
Contributor Author

MakDon commented Apr 4, 2019

@serhiy-storchaka
It was modified from ('a.b(?s)', 'a\nb', SUCCEED, 'found', 'a\nb')
After the commit bd48d27 , flags should be used first in the expression string.
So it was edited to (?s)a.b, which was duplicated.
A new commit has made in which i replace \n with \r

The Azure Pipeline PR was failed because GitHub reported the error, "Request timed out after 100000 ms".I am not familiar with it and i dont know how to deal with it.
Thanks for your review.

@serhiy-storchaka serhiy-storchaka added skip issue skip news tests Tests in the Lib/test dir labels Apr 4, 2019
@serhiy-storchaka serhiy-storchaka merged commit ded4737 into python:master Apr 4, 2019
@serhiy-storchaka
Copy link
Member

Thank you for your contribution @MakDon!

@tirkarthi
Copy link
Member

tirkarthi commented Apr 4, 2019

I can see one more copy of ('(?s)a.b', 'a\nb', SUCCEED, 'found', 'a\nb') at https://github.com/MakDon/cpython/blob/26e169ca686b66ec87d76fa907a253fccdcddcc7/Lib/test/re_tests.py#L592 which I think is the same. Also should this be backported to 3.7 ?

@tirkarthi
Copy link
Member

tirkarthi commented Apr 4, 2019

@serhiy-storchaka Does this file in general have duplicates as part of the same list for some reason?

('ab*', 'xayabbbz', SUCCEED, 'found', 'a'),

('ab*', 'xayabbbz', SUCCEED, 'found', 'a'),

Adding below to the file I can see some more duplicates :

test_map = {}

for test in tests:
    key = test[0] + test[1]
    if key not in test_map:
        test_map[key] = None
    else:
        print(f"duplicate {test[0]} {test[1]}")

print("Set returns ", len(set(map(lambda x: (x[0] + x[1]), tests))))
print("List returns ", len(tests))
Set returns  396
List returns  508

@MakDon
Copy link
Contributor Author

MakDon commented Apr 4, 2019

It seems that this tests are combined from several parts such as # All tests from Perl, which cause the duplicated tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip issue skip news tests Tests in the Lib/test dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants