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

Makes functional tests auto-update works even if the current output is wrong #6891

Conversation

Pierre-Sassoulas
Copy link
Member

Type of Changes

Type
🔨 Refactoring

Description

Previously if an exception was raised during parsing we re-raised an explanation.
Most of the time I just want to regenerate the output from the current pylint's
output. It's what the exception suggested but it's impossible to do as the error
is also raised when updating, so you had to empty the file.

@Pierre-Sassoulas Pierre-Sassoulas added the Maintenance Discussion or action around maintaining pylint or the dev workflow label Jun 8, 2022
@Pierre-Sassoulas Pierre-Sassoulas force-pushed the functional-update-even-for-bogus-expected-output branch from 8f4b536 to 437acf5 Compare June 8, 2022 11:01
@coveralls
Copy link

coveralls commented Jun 8, 2022

Pull Request Test Coverage Report for Build 2486769284

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.004%) to 95.534%

Totals Coverage Status
Change from base Build 2484629988: -0.004%
Covered Lines: 16387
Relevant Lines: 17153

💛 - Coveralls

@github-actions

This comment has been minimized.

Copy link
Collaborator

@DanielNoord DanielNoord left a comment

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 the use case.

If the test fails how can we know what the expected output is?

@Pierre-Sassoulas
Copy link
Member Author

I don't really understand the use case.

You're rebasing a branch with change in the functional expected output. You don't want to fix the merge conflict, you just want to regenerate with the update option. Right now the update option fail if the output contain >>>>>, with a message telling you to use the update option. This adds a warning (with a 3.0 compatible format insteaf of the old ones) instead of an error so the update will work, and the non update will work as if there was nothing in the file instead of a bogus output.

@DanielNoord
Copy link
Collaborator

Okay, but simply discarding the merge change also works right?

With the new code we only raise an UserWarning on actual malformed lines. I don't think that's a good change.

@Pierre-Sassoulas
Copy link
Member Author

Pierre-Sassoulas commented Jun 12, 2022

Okay, but simply discarding the merge change also works right?

Sure, but it's more work, and the message telling us to use update is wrong.

With the new code we only raise an UserWarning on actual malformed lines. I don't think that's a good change.

The functional test will also fail and ask you to update because we return cls("", 0, 0, None, None, "", "", "") which will never be valid.

Copy link
Collaborator

@DanielNoord DanielNoord left a comment

Choose a reason for hiding this comment

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

The functional test will also fail and ask you to update because we return cls("", 0, 0, None, None, "", "", "") which will never be valid.

👍

But let's add a changelog entry in the Internal changes section.

…ent output is wrong

Previously if an exception was raised during parsing we re-raised an explanation.
Most of the time I just want to regenerate the output from the current pylint's
output. It's what the exception suggested but it's impossible to do as the error
is also raised when updating, so you had to empty the file.
@Pierre-Sassoulas Pierre-Sassoulas force-pushed the functional-update-even-for-bogus-expected-output branch from 437acf5 to 2d9688f Compare June 13, 2022 06:00
@@ -61,3 +61,8 @@ Internal changes
* ``pylint.testutils.primer`` is now a private API.

Refs #6905

* Fixed an issue where it was impossible to regenerate functional tests output when they existing
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* Fixed an issue where it was impossible to regenerate functional tests output when they existing
* Fixed an issue where it was impossible to regenerate functional tests output when the existing

@Pierre-Sassoulas Pierre-Sassoulas force-pushed the functional-update-even-for-bogus-expected-output branch from 2d9688f to 597f6bf Compare June 13, 2022 06:02
* Fixed an issue where it was impossible to update functional tests output when the existing
output was impossible to parse. Instead of raising an error we raise a warning message and
let the functional test fail with a default value.

Copy link
Collaborator

Choose a reason for hiding this comment

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

?

This is the PR right?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no issue.

Copy link
Collaborator

@DanielNoord DanielNoord left a comment

Choose a reason for hiding this comment

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

We never put PR numbers in the changelog right? If there isn't an issue we just leave that line empty.

doc/whatsnew/2/2.15/index.rst Outdated Show resolved Hide resolved
@github-actions

This comment has been minimized.

Co-authored-by: Daniël van Noord <13665637+DanielNoord@users.noreply.github.com>
@Pierre-Sassoulas Pierre-Sassoulas merged commit 27d91cf into pylint-dev:main Jun 13, 2022
@Pierre-Sassoulas Pierre-Sassoulas deleted the functional-update-even-for-bogus-expected-output branch June 13, 2022 07:33
@github-actions
Copy link
Contributor

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit 02998c4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintenance Discussion or action around maintaining pylint or the dev workflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants