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

Specify the PG TS config 'english' on search #337

Merged
merged 1 commit into from
May 9, 2023

Conversation

bickelj
Copy link
Contributor

@bickelj bickelj commented May 5, 2023

While the postgres (PG) text search (TS) config 'english' is specified when building the text search column and index in the migration script 0010-alter-proposal_field_values-value_search.sql, when the postgres default_text_search_config is set to pg_catalog.simple rather than to pg_catalog.english, the test named returns a subset of proposals present in the database when search is provided in file proposals.int.test.ts will fail. For consistency in search results across instances, this commit sets the query with search to use the same english. Without this commit, tests will fail with different PG settings for default text search config and inconsistent search results will appear between instances.

Issue #336 Test failure on different pg text search settings

@bickelj bickelj requested review from slifty and jasonaowen May 5, 2023 16:20
@codecov
Copy link

codecov bot commented May 5, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (308045f) 96.12% compared to head (faa71cb) 96.12%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #337   +/-   ##
=======================================
  Coverage   96.12%   96.12%           
=======================================
  Files          65       65           
  Lines         929      929           
  Branches      159      159           
=======================================
  Hits          893      893           
  Misses         35       35           
  Partials        1        1           

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@slifty slifty left a comment

Choose a reason for hiding this comment

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

great catch! (and hooray for tests!)

Out of curiosity... would it be possible to easily write an integration test that ensures this works when pg_catalog.simple is the configured setting? It would be worth including a regression test if so. If not, that's fine too.

@bickelj bickelj force-pushed the use-english-config-on-select-with-search branch from c783a51 to 58f019b Compare May 5, 2023 19:52
@bickelj
Copy link
Contributor Author

bickelj commented May 5, 2023

It was pretty easy to add a single statement to the existing test, setting the default config to 'arabic', which then causes the test to fail if 'english' is not specified in the select query. Does this look OK?

@bickelj bickelj force-pushed the use-english-config-on-select-with-search branch from 58f019b to 838f01d Compare May 5, 2023 19:54
@bickelj
Copy link
Contributor Author

bickelj commented May 5, 2023

Another thought I had was to use the exact bitnami image in our test that we use in prod. We don't have a non-manual way to synchronize that, however, and there is utility in testing with a recent postgres image. If I had to choose one or the other I'd test using the exact image we use in prod, but perhaps we should do both. What do you think, @slifty @jasonaowen ?

@bickelj bickelj requested a review from slifty May 5, 2023 20:11
Copy link
Contributor

@jasonaowen jasonaowen left a comment

Choose a reason for hiding this comment

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

Good digging, @bickelj! Thanks for fixing this.

src/__tests__/proposals.int.test.ts Outdated Show resolved Hide resolved
@jasonaowen
Copy link
Contributor

Another thought I had was to use the exact bitnami image in our test that we use in prod. We don't have a non-manual way to synchronize that, however, and there is utility in testing with a recent postgres image. If I had to choose one or the other I'd test using the exact image we use in prod, but perhaps we should do both. What do you think, @slifty @jasonaowen ?

Well, I have a few thoughts, in no particular order:

  • we should have alignment between dev and prod, but maybe we should move in the other direction - can we stop using the bitnami image altogether? It's already varied from our expectations in a way that impacted functionality; will it do so again?
  • we could run the tests against multiple versions with a matrix strategy
  • rather than running a docker image, or running our own PostgreSQL at all, maybe we should be using Heroku or Digital Ocean; their tooling is better than anything we can build with the budget we have, and our engineering time is better spent on the application itself rather than on hosting

While the postgres (PG) text search (TS) config 'english' is specified
when building the text search column and index in the migration script
`0010-alter-proposal_field_values-value_search.sql`, when the postgres
`default_text_search_config` is set to `pg_catalog.simple` rather than
to `pg_catalog.english`, the test named `returns a subset of proposals
present in the database when search is provided` in file
`proposals.int.test.ts` will fail. For consistency in search results
across instances, this commit sets the query with search to use the
same `english`. Without this commit, tests will fail with different
PG settings for default text search config and inconsistent search
results will appear between instances.

Issue #336 Test failure on different pg text search settings
@bickelj bickelj force-pushed the use-english-config-on-select-with-search branch from 838f01d to faa71cb Compare May 8, 2023 19:39
@bickelj
Copy link
Contributor Author

bickelj commented May 8, 2023

I rebased on main and implemented the suggestion of splitting out the regression test from the original test.

Jason said:

  • we should have alignment between dev and prod, but maybe we should move in the other direction - can we stop using the bitnami image altogether? It's already varied from our expectations in a way that impacted functionality; will it do so again?

  • we could run the tests against multiple versions with a matrix strategy

  • rather than running a docker image, or running our own PostgreSQL at all, maybe we should be using Heroku or Digital Ocean; their tooling is better than anything we can build with the budget we have, and our engineering time is better spent on the application itself rather than on hosting

I like the matrix strategy. Multiple versions would be good, e.g. "what we use in prod today" as well as "a latest version", and/or a "stock pg image" and a "bitnami pg image".

I looked for the download button for Heroku and couldn't figure out how to download a Heroku 😃

Digital Ocean's default configuration also sets simple:

defaultdb=> select name, setting from pg_settings where name like '%default_text%';
            name            |      setting      
----------------------------+-------------------
 default_text_search_config | pg_catalog.simple
(1 row)

Therefore we likely would have run into the same issue with Digital Ocean as the bitnami image. I don't know Heroku's default setting.

Regardless, I think we agree that we should align the tests one way or another. Because we already use containers in test and prod and because GitHub already uses containers during workflows I still think the best path is to use that image during the workflow.

@bickelj bickelj requested a review from jasonaowen May 8, 2023 20:17
Copy link
Contributor

@jasonaowen jasonaowen left a comment

Choose a reason for hiding this comment

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

Thanks for splitting that test out!

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

Successfully merging this pull request may close these issues.

None yet

3 participants