test: silence BigQuery IAM/schema noise from background mocks#3550
test: silence BigQuery IAM/schema noise from background mocks#3550nelsonmestevao wants to merge 1 commit into
Conversation
| test "updates correctly" do | ||
| prev_config = Application.get_env(:logflare, Schema) | ||
| Application.put_env(:logflare, Schema, updates_per_minute: 1) | ||
| on_exit(fn -> Application.put_env(:logflare, Schema, prev_config) end) |
There was a problem hiding this comment.
Force updates_per_minute: 1 so the Schema throttle actually engages. With the test config's 900_000, the throttle window is ~0ms, so the second update raced past expect(:bigquery_tables_patch, 1) and crashed the GenServer. This way we avoid flakyness introduced by the crash.
c13fe35 to
fb434c5
Compare
1a05382 to
e6bfab2
Compare
524d14d to
90f65c2
Compare
2383e77 to
b856ceb
Compare
amokan
left a comment
amokan
left a comment
There was a problem hiding this comment.
I think we should keep the shared stubs, but prob need to revisit some of the expects we removed from things like users_test.exs and single_tenant_test.exs unless I'm missing something bigger picture as these changes seem to cost us some coverage. Totally a fan of reducing noise, but my hope is that we can do so without sacrificing legit test cases 🙏
Part of me also wonders if the blanket stub should be in the data case at all or if it would be better suited as a narrower opt-in for the test files that produce the most noise.
| func.() | ||
| end) | ||
|
|
||
| stub(BigQueryAdaptor, :update_iam_policy, fn -> :ok end) |
There was a problem hiding this comment.
Robot reviewer callout 🤖
The blanket update_iam_policy stub breaks 4 existing tests that exercise the real code path (CI-confirmed). These tests were not given the call_original escape hatch that the controller tests got, so the stub swallows the call before it reaches the GoogleApi mocks, and their expects fail at verify_on_exit!. Failing in this PR's CI run: test/logflare/users/users_test.exs:47 (delete_user/1), test/logflare/partners_test.exs:80, and test/logflare/backends/bigquery_adaptor_test.exs:501 and :568 — the latter two are the unit tests of update_iam_policy/0 itself, which now test the stub rather than the adaptor.
There was a problem hiding this comment.
I fixed those in last push! 🙏
The idea is to never think about the update_iam_policy noise in the logs and only do the call_original when we really are testing these.
cc/ @Ziinc that requested this #3484 (comment)
7dff0ff to
855aed1
Compare
Tests were logging (and occasionally failing on) Mimic errors caused by BigQuery calls. By globally stubbing `BigQueryAdaptor` calls to update_iam_policy/0,1 and patch_dataset_access/1 we reduce the noise.
855aed1 to
588b5d7
Compare
2 participants
Tests were logging (and occasionally failing on) Mimic errors caused by BigQuery calls. By globally stubbing
BigQueryAdaptorcalls to update_iam_policy/0,1 and patch_dataset_access/1 we reduce the noise.