[HOLD for payment 2024-09-16][$250] [Search v2.1] Create automated tests for the Search parser

[luacmartins]

Problem

We introduced a search syntax grammar and generated a parser for it, but any changes to the grammar could affect existing functionlity.

Solution

Create a set of automated tests to validate that input queries translate to the correct AST response when parsed by searchParser.parse

We can find a set of test cases in this file

The test case should be extensible so that we can easily add more query/result examples to it.

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~012a2286768945350c
• Upwork Job ID: 1829237121495398264
• Last Price Increase: 2024-08-29
• Automatic offers:
• ikevin127 | Contributor | 103740228

Issue Owner

Current Issue Owner: @VictoriaExpensify

[melvin-bot[bot]]

Triggered auto assignment to @VictoriaExpensify (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details. Please add this Feature request to a GH project, as outlined in the SO.

[melvin-bot[bot]]

Job added to Upwork: https://www.upwork.com/jobs/~012a2286768945350c

[melvin-bot[bot]]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @c3024 (External)

[melvin-bot[bot]]

📣 @ikevin127 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

[ikevin127]

@luacmartins I'm assuming I should've been assigned as C+ reviewer here ?

[jaydamani]

Edited by proposal-police: This proposal was edited at 2024-08-30 15:05:49 UTC.

Proposal

Please re-state the problem that we are trying to solve in this issue.

Create test files for search parsers

What is the root cause of that problem?

NA

What changes do you think we should make in order to solve the problem?

Create SearchParserTest.ts in tests/unit with all the test cases from the document. The tests can written in below ways:

Option 1:

Considering that most test cases have a very deep nesting of just and operator, we can write a function tranformFilters that will transform a simplified version of the filter into the ast for the query

const query = "las vegas amount:100"
const filters = {
    op: 'and',
    values: [
        {
            op: 'eq',
            left: 'amount',
            right: 100
        },
        {
            op: 'or',
            values: [
                {
                    op: 'eq',
                    left: 'keyword',
                    right: 'vegas'
                },
                {
                    op: 'eq',
                    left: 'keyword',
                    right: 'las'
                }
            ]
        }
    ]
}

const result = {
    type: 'expense',
    status: 'all',
    sortBy: 'date',
    sortOrder: 'desc',
    filters: transformFilters(filters)
}

expect(searchParser.parse(query)).toEqual(result)

Option 2

We can also represent filters in the below way which is much easier to read but adds some complexity to tests whcih may not be wanted

const query = "from:usera@user.com to:userb@user.com date>2024-12-12 keyword1 keyword2"
const filters = {
    from: 'usera@user.com',
    to: 'userb@user.com',
    date: {
      op: 'gt',
      value: '2024-12-12'
    }
    keywords: ['keyword1', 'keyword2'],
};

const result = {
    type: 'expense',
    status: 'all',
    sortBy: 'date',
    sortOrder: 'desc',
    filters: transformFilters(filters)
}

expect(searchParser.parse(query)).toEqual(result)

Option 3

For multiple tests, we can use below approach and this can also be combined with the above options. (with suggestion of test.each from @Kicu)

const tests = [{
    query: "type:expense status:all",
    expected: {
        type: 'expense',
        status: 'all',
        sortBy: 'date',
        sortOrder: 'desc',
        filters: null
      }      
}, { 
    query: "in:123333 currency:USD merchant:marriott",
    expected: {
        type: 'expense',
        status: 'all',
        sortBy: 'date',
        sortOrder: 'desc',
        filters: {
            operator: 'and',
            left: {
                operator: 'and',
                left: {
                    operator: 'eq',
                    left: 'in',
                    right: '123333'
                },
                right: {
                    operator: 'eq',
                    left: 'currency',
                    right: 'USD'
                }
            },
            right: {
                operator: 'eq',
                left: 'merchant',
                right: 'marriott'
            }
        }
    }
}]

describe("search parser", () => {
  test.each(tests)(`parsing: $query`, ({ query, expected }) => {
      const result = parse(query);
      expect(result).toEqual(expected);
  });
});

[jaydamani]

@luacmartins not sure where to put this but while checking the searchParser.peggy file, I found a bug where if you search for words that start with special words like incident, it gives below output which is incorrect

{
  ...
  filters: {
    operator: 'eq',
    left: 'in',
    right: 'cident'
  }
}

Tested this by pasting searchParser.peggy in this site and then giving inputs like incident, tomato or anything that starts with the filter keys Please ping me if this is opened as a separate issue

[luacmartins]

@jaydamani I can't reproduce that with the latest syntax

[luacmartins]

@jaydamani the proposal seems good, but can you expand on how we'd handle the many different test cases in a scalable way?

[jaydamani]

Since filters are the biggest concern in our test cases, we can do following to simplify them:

Option 1:

Considering that most test cases have a very deep nesting of just and operator, we can write a function tranformFilters that will transform a simplified version of the filter into the ast for the query

const query = "las vegas amount:100"
const filters = {
    op: 'and',
    values: [
        {
            op: 'eq',
            left: 'amount',
            right: 100
        },
        {
            op: 'or',
            values: [
                {
                    op: 'eq',
                    left: 'keyword',
                    right: 'vegas'
                },
                {
                    op: 'eq',
                    left: 'keyword',
                    right: 'las'
                }
            ]
        }
    ]
}

const result = {
    type: 'expense',
    status: 'all',
    sortBy: 'date',
    sortOrder: 'desc',
    filters: transformFilters(filters)
}

expect(searchParser.parse(query)).toEqual(result)

Option 2

Continuing from above thinking, we can also represent filters in the below way but this adds too much abstraction between the actual filter and our representation

const query = "from:usera@user.com to:userb@user.com "
const filters = {
    from: 'usera@user.com',
    to: 'userb@user.com',
    date: {
      op: 'gt',
      value: '2024-12-12'
    }
    keywords: ['keyword1', 'keyword2'],
};

const result = {
    type: 'expense',
    status: 'all',
    sortBy: 'date',
    sortOrder: 'desc',
    filters: transformFilters(filters)
}

expect(searchParser.parse(query)).toEqual(result)

Option 3

If you don't want any complex logic in the test and the only concern is number of tests, we can create an object at top or a json file that looks like below and the tests will just loop the array

[{
    query: "type:expense status:all",
    result: {
        type: 'expense',
        status: 'all',
        sortBy: 'date',
        sortOrder: 'desc',
        filters: null
      }      
}, { 
    query: "in:123333 currency:USD merchant:marriott",
    result: {
        type: 'expense',
        status: 'all',
        sortBy: 'date',
        sortOrder: 'desc',
        filters: {
            operator: 'and',
            left: {
                operator: 'and',
                left: {
                    operator: 'eq',
                    left: 'in',
                    right: '123333'
                },
                right: {
                    operator: 'eq',
                    left: 'currency',
                    right: 'USD'
                }
            },
            right: {
                operator: 'eq',
                left: 'merchant',
                right: 'marriott'
            }
        }
    }
}]

looping logic:

describe("search parser", () => {
  tests.forEach(({ query, expected }) => {
    test(`correctly parses "${query}"`, () => {
      const result = parse(query);
      expect(result).toEqual(expected);
    });
  });
});

[jaydamani]

@jaydamani I can't reproduce that with the latest syntax

I tested this on main branch, is there any different branch with latest code for search functionality?

[daledah]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Create automated tests for the Search parser

What is the root cause of that problem?

New Test file

What changes do you think we should make in order to solve the problem?

1. For query input I suggest creating a new buildQuery function. This is my quick draft

function buildQuery(params) {
  const queryParts = [];

  if (params.type) queryParts.push(`type=${params.type}`);
  if (params.status) queryParts.push(`status=${params.status}`);

  if (params.amount && params.amountOperator) {
    queryParts.push(`amount${params.amountOperator || ':'}${params.amount}`);
  }

  if (params.keywords) queryParts.push(params.keywords.join(' '));

  return queryParts.join(' ');
}

we can define the specific type for each params (string, number, Enum, Date,...) when using buildQuery function

params = { type: ENUM.{...}, status: ENUM.{...}, amount: 123,.... }

2. For the result, I think we should use raw output or we also create a buildResult function, then we can set default value for: type, status, sortBy, sortOrder Ex:

   
   function buildResult(params) {
   let output;
   output.type = params.type || 'expense'
   ...

with filters object, I think we should hard string code to make sure the test case work exactly

### What alternative solutions did you explore? (Optional)

[Kicu]

Hello, I work at an expert agency I wanted to add a small suggestion here, about this:

how we'd handle the many different test cases in a scalable way?

There is a dedicated functionality in jest to help with testing multiple test cases: https://jestjs.io/docs/api#each

It might be helpful to use it for our parser unit tests.

[jaydamani]

Updated proposal

• Included details of this comment with better wording
• updated to include suggestion from @Kicu

[luacmartins]

@jaydamani I think option 3 is what we're looking for here.

[melvin-bot[bot]]

📣 @jaydamani You have been assigned to this job! Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs! Keep in mind: Code of Conduct | Contributing 📖

[daledah]

@luacmartins How about my proposal?

[daledah]

I think it will be more extensible so that we can easily add more query/result examples to it.

[jaydamani]

@luacmartins, I will be working on this over the weekend, so the PR should be ready to review by monday.

[luacmartins]

@daledah your proposal introduces a buildQuery method, that we don't need. The whole point of the tests is to test the searchParser.parse function that builds the AST. This test should just confirm that for a given string input, e.g. type:expense status:all, we should get the correct parsed AST, e.g. {type: 'expense', status: 'all', sortBy: 'date', sortOrder: 'desc', filters: null}. The extensibility needed is just us adding more input/expectedValues to the test case without needing to change the test logic.

[jaydamani]

Created this PR with comment for failing test cases

[VictoriaExpensify]

Not overdue, payment due on 16th

[ikevin127]

cc @VictoriaExpensify