Skip to content

Conversation

@flvndh
Copy link
Contributor

@flvndh flvndh commented Sep 18, 2020

Tackle #10995

@boring-cyborg boring-cyborg bot added the provider:microsoft-azure Azure-related issues label Sep 18, 2020
@flvndh flvndh force-pushed the issue/10995/azure-data-factory branch 3 times, most recently from 59ae178 to 20f05f6 Compare September 22, 2020 16:01
@flvndh flvndh marked this pull request as ready for review September 22, 2020 16:13
Copy link
Member

@feluelle feluelle left a comment

Choose a reason for hiding this comment

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

Thank you for all the tests @flvndh 👍 maybe you want to reduce the code but still keep the great coverage.. (see below)

@flvndh flvndh force-pushed the issue/10995/azure-data-factory branch from 20f05f6 to e35e19a Compare October 19, 2020 14:55
@flvndh
Copy link
Contributor Author

flvndh commented Oct 19, 2020

Thank you very much @feluelle for your review. I've applied all your comments. Note that parameterized seems to be incompatible with the latest versions of pytest. What do you think of using @pytest.mark.parametrize instead?

@feluelle
Copy link
Member

Oh really? What error do you get? @pytest.mark.parametrize seems also to be a good option to use :)

@flvndh
Copy link
Contributor Author

flvndh commented Oct 19, 2020

Tests are ignored and pytest emits a PytestCollectionWarning stating that "yield tests are no more collected".

How do you see the tests being refactored? I had in mind one test per method and use parameterization for the implicit and explicit factory cases. It Is also possible to go far and have one or two hyper parameterized functions but I think it will make the tests less readable. What do you think?

@feluelle
Copy link
Member

I had in mind one test per method and use parameterization for the implicit and explicit factory cases.

I like this. 👍

It Is also possible to go far and have one or two hyper parameterized functions but I think it will make the tests less readable.

Readability is far more important than code duplication in my opinion. So I am in favor for one test per method.

@github-actions
Copy link

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks$,^Build docs$,^Spell check docs$,^Backport packages$,^Checks: Helm tests$,^Test OpenAPI*.

@github-actions
Copy link

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks$,^Build docs$,^Spell check docs$,^Backport packages$,^Checks: Helm tests$,^Test OpenAPI*.

@github-actions
Copy link

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@github-actions
Copy link

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@github-actions
Copy link

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@feluelle
Copy link
Member

feluelle commented Nov 3, 2020

LGTM. Can you resolve the failing static checks, please? :)

@flvndh flvndh force-pushed the issue/10995/azure-data-factory branch from ae7d9e0 to 908cf16 Compare November 3, 2020 20:15
@github-actions
Copy link

github-actions bot commented Nov 3, 2020

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@kaxil
Copy link
Member

kaxil commented Nov 4, 2020

Can you please rebase your PR on latest Master since we have applied Black and PyUpgrade on Master.

It will help if your squash your commits into single commit first so that there are less conflicts.

@flvndh flvndh force-pushed the issue/10995/azure-data-factory branch from 908cf16 to c574525 Compare November 4, 2020 12:01
@github-actions
Copy link

github-actions bot commented Nov 4, 2020

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@stale
Copy link

stale bot commented Dec 25, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Dec 25, 2020
@kaxil
Copy link
Member

kaxil commented Jan 3, 2021

@flvndh Can you please resolve the conflicts and ping us again, thanks

@stale stale bot removed the stale Stale PRs per the .github/workflows/stale.yml policy file label Jan 3, 2021
@mivinson
Copy link

@flvndh Can you please look at the merge conflicts? This is a very useful package that you have authored.

@github-actions
Copy link

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@github-actions
Copy link

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@flvndh flvndh force-pushed the issue/10995/azure-data-factory branch from f46101d to d1681a6 Compare February 22, 2021 10:04
@flvndh
Copy link
Contributor Author

flvndh commented Feb 22, 2021

@don4of4 Here you go 😀

@don4of4
Copy link

don4of4 commented Feb 24, 2021

@kaxil Can we get this reviewed?

@kaxil
Copy link
Member

kaxil commented Feb 24, 2021

Let's wait for the CI before we merge this

@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Feb 24, 2021
@github-actions
Copy link

The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest master at your convenience, or amend the last commit of the PR, and push it with --force-with-lease.

@github-actions
Copy link

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@ShawnMcGough
Copy link

@kaxil - does this need to be re-queued or just waiting for the reviewers?

Thanks!

@kaxil kaxil force-pushed the issue/10995/azure-data-factory branch from d1681a6 to f784ecb Compare February 25, 2021 18:58
@kaxil
Copy link
Member

kaxil commented Feb 25, 2021

The PR was some 200 commits behind of Master -- so rebased and pushed --let's wait for the CI results again

@ShawnMcGough
Copy link

@kaxil looks like a doc build failed and two MySQL/Py builds. 🤔

@kaxil kaxil merged commit 11d03d2 into apache:master Feb 26, 2021
@kaxil
Copy link
Member

kaxil commented Feb 26, 2021

@kaxil looks like a doc build failed and two MySQL/Py builds. 🤔

Transient failures :)

potiuk pushed a commit that referenced this pull request Mar 3, 2021
fixes #10995

(cherry picked from commit 11d03d2)
potiuk pushed a commit that referenced this pull request Mar 3, 2021
fixes #10995

(cherry picked from commit 11d03d2)
potiuk pushed a commit that referenced this pull request Mar 3, 2021
fixes #10995

(cherry picked from commit 11d03d2)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

full tests needed We need to run full set of tests for this PR to merge provider:microsoft-azure Azure-related issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants