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

Fix smoke tests parsing issue by adding smoke_tests.sql and husky pre… #60

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

sanketdisale871
Copy link

Pull Request Description:

Fix Smoke Tests Parsing Issue

This pull request addresses issue #55.

Issue Description:

Currently, the smoke tests included in the workflow can lead to parsing issues when updates are made. This occurs due to the absence of a dedicated file for smoke tests and lack of validation for its SQL syntax.

Solution:

To resolve this issue, the following changes have been implemented:

  1. Created a new .sql file named smoke_tests.sql specifically for smoke tests.
  2. Added a husky pre-commit hook to validate the syntax of the smoke_tests.sql file.

The husky pre-commit hook ensures that the smoke_tests.sql file contains valid SQL syntax before committing any changes. This proactive validation helps to avoid parsing issues and ensures the integrity of the smoke tests.

Please review these changes and provide your feedback.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need this file ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need this file ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need this file ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

@jayanth-kumar-morem jayanth-kumar-morem left a comment

Choose a reason for hiding this comment

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

Could you please add comments to your code? It would help others understand the functionality better.

@jayanth-kumar-morem
Copy link
Collaborator

jayanth-kumar-morem commented Jun 10, 2023

Thank you @sanketdisale871 for your work on this pull request. The addition of a dedicated .sql file for smoke tests and the implementation of a husky pre-commit hook for SQL syntax validation are great improvements.

To fully integrate these changes into our workflow, could you please also update our GitHub Actions workflow to use the new smoke_tests.sql file for running smoke tests? This will ensure that the tests are executed consistently across all environments and that the SQL syntax validation is applied during our automated testing process.

Please let me know if you need any assistance with this. Thanks again for your contribution.

CC: @ChakshuGautam

@sanketdisale871
Copy link
Author

I need assistance.

@jayanth-kumar-morem
Copy link
Collaborator

Sure @sanketdisale871

Instead of writing psql -c "select ..." multiple times in our GHA workflow. We can move all those queries to test the extensions to an sqlfile and pass the sqlfile directly to GHA.

Here's a sample code

      - name: Run the smoke test
        run: |
          set -eu
          export PGHOST=localhost
          export PGUSER=postgres
          export PGPASSWORD=test1234
          docker container stop smoketest-container || true
          docker container rm smoketest-container || true
          docker run -d -p 5432:5432 -e POSTGRES_PASSWORD=${PGPASSWORD} --name smoketest-container smoketest-image
          for _ in {1..120}
          do
            if [ -z "$(docker container ls -q --filter name=smoketest-container)" ]
            then
              echo "Smoketest container is not running"
              exit 1
            fi
            if psql -f /path/to/our/sqlfile.sql
            then
              break
            fi
            sleep 1
          done
          if ! psql -f /path/to/our/sqlfile.sql
          then
            echo "Cannot connect to PostgreSQL"
            exit 1
          fi

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

All the SQL queries you have deleted from github/workflows/smoke-test.yml should add back to this file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants