FFL-2783: add tests for observeFullEvaluationData PII protection (gated as missing_feature)#7316
Conversation
Adds three non-blocking test classes covering the truth-table matrix for the new per-env UFC field: absent (backcompat), false (explicit disable), and true. Asserts hashed sha256_-prefixed targeting_key + omitted context.evaluation for the first two cells, and raw targeting_key + full context.evaluation for the third. Uses a canonical PII vector (jane.doe@datadoghq.com) so every SDK's L1 unit tests can assert against the same known hash output. All classes gated missing_feature (FFL-2783) on Go; other SDK manifests already skip the whole file. Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
|
|
🎉 All green!🧪 All tests passed 🔗 Commit SHA: a210c39 | Docs | Datadog PR Page | Give us feedback! |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1ad980b2f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| rc.tracer_rc_state.reset().set_config( | ||
| f"{RC_PATH}/{config_id}/config", | ||
| make_ufc_fixture(self.flag_key), | ||
| ).apply() |
There was a problem hiding this comment.
These new setup_* methods perform remote-config application and the weblog request before the test body; if either path raises or times out in CI, pytest reports a setup error and the scenario can stop instead of recording a normal test failure. .cursor/rules/pr-review.mdc says setup methods must only trigger observability and failures/assertions belong in test_*, so keep setup minimal and move the fallible validation path into the matching test method.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not addressing this one. The other eight test classes in this file (Test_FFE_EVP_Flagevaluation_Basic, _Count, _Context_Bounds, _Runtime_Default, _Load_Aggregation, _Burst_Aggregation, _High_Cardinality_Aggregation, _Degradation) all place rc...apply() and evaluate_flag(...) in setup_* and only run assert self.r.status_code == 200 in test_*. Diverging in only the three new classes would create an inconsistency reviewers would flag; if the setup-vs-test split needs enforcement here, it belongs in a dedicated refactor across the whole file.
vjfridge
changed the title
vjfridge
changed the title
The three new missing_feature rows should reference FFL-2784 (Go SDK implementation) rather than FFL-2783 (this L3 test ticket) so an SDK dev lands directly on the work that unblocks activation. Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
- Rename CANONICAL_* to PII_* and move constants + helpers into their existing sections (top-of-file constants, mid-file helpers) for consistency with the rest of the file. - Iterate every matching flagevaluation event in all three PII test classes so a clean first event cannot mask a leaky later event. - Add assert_no_raw_pii_in_event helper that walks the serialized event and asserts none of the raw PII values (targeting key or attributes) appear anywhere — guards against SDKs that route raw PII into unexpected fields (e.g., context.user_email) even when context.evaluation is correctly omitted. Applied to both hashed cells. Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
cbeauchesne
left a comment
cbeauchesne
left a comment
There was a problem hiding this comment.
From framework usage, just a small request change.
Cold you also get a review from someone familiar with the feature for the test logic ?
|
|
||
| @scenarios.feature_flagging_and_experimentation | ||
| @features.feature_flags_evp_flagevaluation | ||
| @pytest.mark.skip_if_xfail |
There was a problem hiding this comment.
There are semantic decorators in utils that allow to achieve the same, and also declare why we want to skip them : @slow and @scenario_crash, could you use them ?
2 participants
Motivation
FFL-2783 — part of FFL-2780 (Protecting PII in flagevaluations track)
See informal RFC for more context
Summary
tests/ffe/test_flag_eval_evp.pycovering the new per-env UFCobserveFullEvaluationDatafieldmake_ufc_fixturewith anobserve_full_evaluation_data: bool | None = Nonekwarg.Noneomits the field entirely, so existing tests are unaffectedmissing_feature (FFL-2783)on Go — the only SDK where the file-level gate would otherwise activate them. Other SDK manifests already deactivate the whole file viamissing_feature (FFL-2446)orirrelevant.Truth-table coverage
observeFullEvaluationDatatargeting_keycontext.evaluationsha256_-prefixed (71 chars, lowercase hex suffix)falsesha256_-prefixedtrueCanonical PII vector (shared across SDKs)
Every SDK's unit tests should assert against the same input/output. Input:
"jane.doe@datadoghq.com"→"sha256_b4698f9b6d186781fa8dc59e533578fa2d8379a46b1cf6db85cda6aa9c99e51b". Canonical attributes cover string, integer, dotted-key, and email PII shapes.Forward-compat for existing SDKs
Introducing a new field inside the UFC
environmentobject could theoretically break SDKs if their UFC parser rejects unknown fields. That risk is already covered bytests/ffe/test_dynamic_evaluation.py::Test_FFE_Unknown_Fields_Tolerance, which injects random unknown fields at every UFC level (including insideenvironment) and asserts each SDK continues to evaluate flags correctly. That test runs on every SDK wheretest_dynamic_evaluation.pyis active, so no per-fixture retrofit is needed here.Out of scope
DD_FLAGGING_EVALUATION_COUNTS_ENABLED) — per-SDK unit test territory, nosystem-testsscenario variant needed.Test plan
./format.shclean (mypy, ruff, node linters)missing_feature (FFL-2783)rows tov2.X.Y-devand confirm all three cells pass🤖 Generated with Claude Code