Yicong-Huang commented on PR #6016: URL: https://github.com/apache/texera/pull/6016#issuecomment-4990801778
Came in a bit late to the discussion. I was tied up with other projects. @carloea2 I appreciate you emphasizing test coverage. That’s definitely the goal: improve coverage and prevent unexpected regressions. I’m not sure where you got the impression that I was approving the PR without tests. As I mentioned earlier, I actually encouraged @Mrudhulraj to add test cases. @Mrudhulraj, thanks for the detailed analysis! I don’t think you need DatasetResource to test this fix. A unit test in the dashboard service should be sufficient: mock the dataset DB records and verify (or spy on) the generated SQL queries. My understanding is that you were planning to create real datasets (in db) through DatasetResource and then verify the dashboard search endpoint. That would effectively become an end-to-end integration test spanning multiple services with a real PostgreSQL instance. I believe this is the test scenario @carloea2 was describing, but we don’t have the infrastructure to support cross-service integration tests yet. In my earlier comments I got confused: I initially thought the codebase wasn’t cleanly separated, by reading your analysis. To help unblock this PR, here’s what I’d suggest: 1. Add some basic unit tests in the dashboard service that focus on the core logic of your fix, mock the database and verify the generated SQL. This is mainly to improve coverage and guard against regressions. 2. Add an integration test for the dashboard service only. Use a real PostgreSQL instance, insert the test records directly in the test (no need to go through the dataset service), and then test the search endpoint. We added CI support for this in #6274. Simply annotate the test with `@IntegrationTest`, and it will have a real Postgres db instance available. 3. We don’t support cross-service integration tests yet. If you think they’re valuable, feel free to open an issue to track the necessary infrastructure work todo. 4. Please move the screenshots into the PR description under the “How Tested” section as supporting evidence. Thanks! Also, @carloea2, if you have bandwidth, please feel free to help @Mrudhulraj with the tests by committing directly to this PR. I can take a look in details tomorrow if I get time. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
