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

Update doctest code for more consistent runs #3053

Merged

Conversation

Swiddis
Copy link
Collaborator

@Swiddis Swiddis commented Oct 4, 2024

Description

Doctest seems to be failing locally because of updates with needClean or pitId fields being included in _explain requests, but when trying to update the test cases to account for this, they start failing again as the field list changes more. This PR adds a normalization step to get these tests to behave more consistently.

Related Issues

Resolves #3048

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • New functionality has javadoc added.
  • New functionality has a user manual doc added.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff.
  • Public documentation issue/PR created.

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.

Copy link
Collaborator

@noCharger noCharger left a comment

Choose a reason for hiding this comment

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

@Swiddis Thanks for the change! Could you verify if this is related #3048?

@Swiddis
Copy link
Collaborator Author

Swiddis commented Oct 7, 2024

@Swiddis Thanks for the change! Could you verify if this is related #3048?

I copied the changes to 2.x and looked at the failures: those failures actually seem related to this PR: updating a specific dependency of sql-cli caused the padding on formatted tables to change. I'll add the updates to this PR.

Signed-off-by: Simeon Widdis <[email protected]>
@Swiddis
Copy link
Collaborator Author

Swiddis commented Oct 7, 2024

I copied the changes to 2.x and looked at the failures: those failures actually seem related to opensearch-project/sql-cli#30: updating a specific dependency of sql-cli caused the padding on formatted tables to change. I'll add the updates to this PR.

So, it turns out that this formatting change affects a lot of tables. I automated refactoring all of them and verified it passes doctest locally, the diff ends up at ~2k lines. To refresh the doctests locally you need to purge the venv and repo with outdated dependencies, rm -rf ./doctest/.venv ./doctest/sql-cli.

@Swiddis Swiddis force-pushed the maintenance/stabilize-doctest branch from f935487 to 7caaff6 Compare October 7, 2024 21:32
@Swiddis
Copy link
Collaborator Author

Swiddis commented Oct 7, 2024

Test failures related to #3056, waiting to merge that

@Swiddis Swiddis requested a review from noCharger October 8, 2024 17:59
@Swiddis
Copy link
Collaborator Author

Swiddis commented Oct 8, 2024

Gradle build is passing as advertised, BWC seems to have unrelated issues (would it be worthwhile to split gradle/bwc tests into separate actions?)


If the passed response is not an _explain response, the input is left unmodified.
"""
def normalize_explain_response(data):
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems all query reponse are impacted, not only the explain query. do u know why the format is different? does it impact rest api customer?

Copy link
Collaborator Author

@Swiddis Swiddis Oct 9, 2024

Choose a reason for hiding this comment

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

Formatting changes because of cli-helpers updating from v1 to v2 in sql-cli, which removes some extra whitespace from most responses. It's separate from the _explain issue.

Copy link
Collaborator

@penghuo penghuo left a comment

Choose a reason for hiding this comment

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

Thx!

@Swiddis Swiddis added v2.18.0 Issues targeting release v2.18.0 infrastructure Changes to infrastructure, testing, CI/CD, pipelines, etc. and removed v2.18.0 Issues targeting release v2.18.0 labels Oct 9, 2024
@penghuo penghuo merged commit cf1564b into opensearch-project:main Oct 11, 2024
16 of 17 checks passed
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/sql/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/sql/backport-2.x
# Create a new branch
git switch --create backport/backport-3053-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 cf1564b6aeb03e49f19032d14bf69d51d6332984
# Push it to GitHub
git push --set-upstream origin backport/backport-3053-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/sql/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-3053-to-2.x.

Copy link
Collaborator

@noCharger noCharger left a comment

Choose a reason for hiding this comment

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

@Swiddis should this backport to 2.x?

Swiddis added a commit to Swiddis/sql that referenced this pull request Oct 21, 2024
Swiddis added a commit that referenced this pull request Oct 25, 2024
* Update doctest code for more consistent runs

Signed-off-by: Simeon Widdis <[email protected]>

* Update doctest code for more consistent runs (#3053)

Signed-off-by: Simeon Widdis <[email protected]>
(cherry picked from commit cf1564b)

* Fix doctests

Signed-off-by: Simeon Widdis <[email protected]>

* Fix MalformedqueryIT

Signed-off-by: Simeon Widdis <[email protected]>

---------

Signed-off-by: Simeon Widdis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x infrastructure Changes to infrastructure, testing, CI/CD, pipelines, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Fix doc test on 2.x
3 participants