Skip to content

Conversation

@MareStare
Copy link
Contributor

@MareStare MareStare commented May 13, 2025

Added the initial plumbing for Philomena Search Syntax parser tests.

The bulk of this code implements the "testing engine" for the parser. The tests are defined declaratively like this:

assert_search_syntax(%{
  compile: &Philomena.Images.Query.compile/2,
  snapshot: "#{__DIR__}/search-syntax.json",
  contexts: %{
    user: [/* ... */],
    watch: [true, false]
  },
  test_cases: [
    wildcard: [
      "*",
      "artist:*",
    ]
  ]
})

This test can be invoked either via the regular mix test to validate that the existing test snapshot is fresh. Or via philomena update-phiql-tests to update the test snapshot after a change was made to the source code. The test snapshot has this format in the simplest form:

{
  "wildcard": [
    {
      "philomena": "*",
      "opensearch": {
        "match_all": {}
      }
    }
  ]
}

There can be an additional contexts key in the test case in case if the parser output differs between contexts like anon user, regular user or admin/mod user.

Todo

Fix this comment from @liamwhite in Discord:

fields which do not exist on the document and are created by a callback

Consider using toml instead of JSON and try define the test inputs directly in the snapshot?

@MareStare MareStare force-pushed the feat/parser-snapshot-tests branch from eb11616 to f14f637 Compare May 13, 2025 02:25
@@ -0,0 +1,138 @@
defmodule Philomena.QueryCase do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Todo rename QueryCase to PhiqlCase

@Meow
Copy link
Member

Meow commented May 13, 2025

De-ponify the test queries (since I assume it doesn't really matter what they are), we're aiming for eventual debranding and more neutral texts in the software in the future.

@Meow
Copy link
Member

Meow commented May 13, 2025

I also think calling it just "query" instead of "phiql" is less confusing. In my mind the language's name has always been just "search syntax" or "query syntax".

@MareStare
Copy link
Contributor Author

Marking as draft until I fix the review comments

@MareStare MareStare marked this pull request as draft May 13, 2025 19:10
@MareStare MareStare changed the title Begin Philomena Query Language parser snapshot tests Begin Philomena Search Syntax parser snapshot tests May 13, 2025
@MareStare MareStare marked this pull request as ready for review May 14, 2025 03:06
@MareStare
Copy link
Contributor Author

MareStare commented May 14, 2025

This is now ready for review

Fixed the review comments and improved the code a bit:

  • Unfortunately, there is practically no TOML encoder library existent in Elixir. But anyway, TOML isn't really good at representing nested data structures. In this case Opensearch query is such a data structure
  • Deponified the test data
  • Renamed PhiQL to "Philomena.SearchSyntax"
  • Added tests for filter_id and some more test scenarios
  • Changed the user_fixtures.ex a bit during the iterations, but then I ended up not using that module because current tests don't depend on the users being in the DB. However, I left the changes I made in user_fixtures.ex since I believe they may be useful in the future

@MareStare MareStare added this to the 1.2 milestone May 14, 2025
@MareStare MareStare force-pushed the feat/parser-snapshot-tests branch from fc1f6c5 to 7a90475 Compare May 15, 2025 00:50
user_fixture(attrs)
|> Users.User.confirm_changeset()
|> Repo.update!()
user_fixture(Map.put(attrs, :confirmed, true))
Copy link
Member

@Meow Meow May 21, 2025

Choose a reason for hiding this comment

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

use |> syntax here (and the next occurence)

attrs
|> Map.put(...)
|> user_fixture()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 4f90db5. Had to rebase to fix merge conflicts

@MareStare MareStare force-pushed the feat/parser-snapshot-tests branch from ae199f5 to 92c4328 Compare May 21, 2025 11:15
@MareStare MareStare force-pushed the feat/parser-snapshot-tests branch from 92c4328 to 4f90db5 Compare May 21, 2025 11:17
@mdashlw mdashlw removed this from the 1.2 milestone Jun 4, 2025
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.

4 participants