Skip to content

Refactor ext/pgsql test #12608

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

Merged
merged 17 commits into from
Nov 6, 2023
Merged

Conversation

KentarouTakeda
Copy link
Contributor

I refactored the ext/pgsql tests based on some recent pull requests.
The content and results remain unchanged. This is just an internal improvement.

With this refactoring, we can expect the following benefits:

  • Test independence:
    Some tests were dependent on the execution order of previous and subsequent tests.
    All tests can now be executed independently.
  • Test parallelism
    One test no longer interferes with another. All tests can be run in parallel.

Specifically, i made the following modifications: For readability, each modification is fixed in a separate commit.

  • Use --CLEAN-- to delete objects at the end of the test.
    faabc3f
  • Moved *.inc to another folder to make filenames easier to read.
    7b507cd
  • Tables used for tests are defined for each test instead of being set all at once in config.inc
    002fe32 293b0b0
  • Use different object names for each test. The name is based on the test file name.
    002fe32 5ab45c7

Ref:

Copy link
Member

@Girgias Girgias 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 working on this!

It has been very annoying to need to run these tests sequentially.

Could you always use uppercase letters for SQL keywords, and CLEAN sections should use an IF EXISTS clause as the table may not be created in the test.

@KentarouTakeda
Copy link
Contributor Author

Could you always use uppercase letters for SQL keywords,

Fixed in 21e61ad

All existing code unrelated to this pull request was also fixed.
However, there were some inconsistencies in the interpretation of what should be treated as "SQL keywords". For those, I respect the original code and have not fixed it.

Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

Looks mostly good now! Only a couple of nits and questions. :)

@KentarouTakeda
Copy link
Contributor Author

@Girgias
Your very detailed review helped me a lot!
I have corrected each of the points pointed out. thank you.

Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

One final remark and good to merge :)

@Girgias Girgias merged commit c15988a into php:master Nov 6, 2023
@Girgias
Copy link
Member

Girgias commented Nov 6, 2023

Thank you!

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

Successfully merging this pull request may close these issues.

2 participants