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

Extract doctest from main CI workflow #915

Conversation

Yury-Fridlyand
Copy link
Collaborator

Description

Doctest depends on ML plugin. If it is unavailable for download for the current version, doctest fails and this ruins all SQL plugin CI.

Issues Resolved

Make SQL plugin CI independent on ML plugin.

Check List

  • New functionality includes testing.
    • All tests pass, including unit test, integration test and doctest
  • New functionality has been documented.
    • New functionality has javadoc added
    • New functionality has user manual doc added
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@Yury-Fridlyand Yury-Fridlyand requested a review from a team as a code owner October 13, 2022 19:15
@Yury-Fridlyand Yury-Fridlyand added the infrastructure Changes to infrastructure, testing, CI/CD, pipelines, etc. label Oct 13, 2022
@codecov-commenter
Copy link

codecov-commenter commented Oct 13, 2022

Codecov Report

Merging #915 (0a8b18d) into 2.x (b30d156) will decrease coverage by 2.80%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##                2.x     #915      +/-   ##
============================================
- Coverage     97.90%   95.10%   -2.81%     
  Complexity     3072     3072              
============================================
  Files           293      303      +10     
  Lines          7588     8246     +658     
  Branches        490      609     +119     
============================================
+ Hits           7429     7842     +413     
- Misses          158      350     +192     
- Partials          1       54      +53     
Flag Coverage Δ
query-workbench 62.76% <ø> (?)
sql-engine 97.90% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ublic/components/QueryResults/QueryResultsBody.tsx 68.32% <0.00%> (ø)
workbench/public/components/app.tsx 0.00% <0.00%> (ø)
workbench/public/components/Main/main.tsx 53.00% <0.00%> (ø)
...h/public/components/QueryLanguageSwitch/Switch.tsx 85.71% <0.00%> (ø)
workbench/public/application.tsx 0.00% <0.00%> (ø)
workbench/public/components/Header/Header.tsx 100.00% <0.00%> (ø)
workbench/public/utils/PanelWrapper.tsx 100.00% <0.00%> (ø)
workbench/public/components/SQLPage/SQLPage.tsx 100.00% <0.00%> (ø)
...ch/public/components/QueryResults/QueryResults.tsx 61.60% <0.00%> (ø)
workbench/public/components/PPLPage/PPLPage.tsx 56.52% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

build.dependsOn doctest
// Deactivate doctest in the main gradle workflow (./gradlew build), because doctest depends
// on ML plugin and it ruins SQL plugin build if ML plugin is not available (not released)
// build.dependsOn doctest
Copy link
Member

Choose a reason for hiding this comment

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

if it's for CI only, maybe disable in workflow instead of removing doctest from build task?

But could you explain more on why? If ml-commons is missing and doctest cannot run, we should not merge in PR anyways since we don't know if the PR breaks other doctests

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we should not merge in PR anyways

I agree. Unfortunately, gradle runs doctests before integration tests and before most of the unit tests. This results in a PR with failed SQL Java CI workflow, which crashed on doctests.
The proposed change unblocks UT and IT from doctest. The PR will remain blocked by a failing CI workflow - SQL Doctest CI. We will be able to identify that all tests really pass and we are waiting for ML release to complete the PR.

@penghuo
Copy link
Collaborator

penghuo commented Oct 26, 2022

I do not think we should disable docTest in IT flow and github flow. The purpose of docTest is make sure the code is align with docs.
If ML/AD is the concern, we could consider disable ML/AD related docTest, but not disable all of them.

@dai-chen
Copy link
Collaborator

+1 for removing ML dependency from doctest. Otherwise, version bump like #898 will always fail and blocked.

@Yury-Fridlyand
Copy link
Collaborator Author

Confirmed. Will do in another PR.

@Yury-Fridlyand Yury-Fridlyand deleted the dev-extract-doctest-from-main-ci branch October 27, 2022 20:51
@Yury-Fridlyand
Copy link
Collaborator Author

#1011

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infrastructure Changes to infrastructure, testing, CI/CD, pipelines, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants