test(ads-client): add integration tests against MARS staging with GitHub CI workflow by Almaju · Pull Request #7275 · mozilla/application-services

Almaju · 2026-03-18T21:00:55Z

Summary

Refactors existing #[ignore] integration tests to use viaduct-hyper and MozAdsEnvironment::Staging for Image, Spoc, and Tile ad types
Adds a GitHub Actions workflow (.github/workflows/ads-client-tests.yaml) that runs these tests on every PR/push touching components/ads-client/**
Run manually with: cargo test -p ads-client --test integration_test -- --ignored

Split from #7263

Pull Request checklist

Breaking changes: This PR follows our breaking change policy

This PR follows the breaking change policy:
- This PR has no breaking API changes, or
- There are corresponding PRs for our consumer applications that resolve the breaking changes and have been approved
Quality: This PR builds and tests run cleanly
- Note:
  - For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
  - If this pull request includes a breaking change, consider cutting a new release after merging.
Tests: This PR includes thorough tests or an explanation of why it does not
Changelog: This PR includes a changelog entry in CHANGELOG.md or an explanation of why it does not need one
- Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
Dependencies: This PR follows our dependency management guidelines
- Any new dependencies are accompanied by a summary of the due diligence applied in selecting them.

Extracts the integration tests into a new `ads-client-integration-tests` crate under `components/ads-client/integration-tests/`. This keeps `viaduct-hyper` out of `ads-client`'s dev-dependencies entirely, since it was only needed to back the HTTP calls in those tests. - Remove `integration-tests` feature flag from `ads-client` - Remove `viaduct-hyper` from `ads-client` dev-dependencies - Add new crate to workspace `members` (not `default-members`) - Update CI workflow to run `cargo test -p ads-client-integration-tests`

…e files

copyrighthero

Executed the unit tests locally and verified that they pass!

It seems that in the new unit tests, we:

Stopped checking the spoc.len() == count, is this intentional?
- Similarly, it seems that instead of testing 3 counts, we are only testing 1 in the new version.
We stopped testing if the placement exists with the placements.get(...).expect(...), I wonder if we still need to?

copyrighthero · 2026-03-24T16:05:05Z

components/ads-client/integration-tests/tests/http_cache.rs

+            },
+        )
+        .unwrap();
+    matches!(o1.cache_outcome, CacheOutcome::MissStored);


Does this matches throw in cases the two values mismatch? Should we assert the returned bool? Similar issue with below match! uses.

assert!(matches!(o1.cache_outcome, CacheOutcome::MissStored), "Expected MissStored, got {:?}", o1.cache_outcome);

Hey, thanks for the feedback! A few clarifications:

Regarding the comment: This code was just moved as-is, so while the observation might be valid, it's out of scope for this MR. Feel free to open a ticket to investigate it further, happy to revisit it separately!

Regarding the HTTP cache integration test: This test is intentionally not part of the regular test suite, it's neither run by cargo test nor by the integration CI. Think of it more as a playground/debug helper: a quick way for us to poke at cache behaviour with a real clock and real URLs if something feels off. We already have a solid set of unit tests covering the cache logic itself, so this is not meant to be a meaningful test gate.

More broadly on the integration test crate: It's best thought of as an external toolbox of debug helpers, living outside of ads-client proper. It's there to help us investigate things in real conditions, not to enforce correctness.

On the staging integration test: Same spirit, it's an optional safety net, not a substitute for unit tests. We already have good coverage at the unit level; the staging test is just a nice-to-have sanity check when we want extra confidence especially against MARS OpenAPI schemas.

Thank you very much for the additional contexts!

I would also add some of these decisions into the README file, for example, the doc says it is testing against the MARS stage while here we are testing against prod.

I understand this is just a simple port, but I don't quite agree with the suggestion that adding in assert! should be done with another ticket/MR due to the fact that:

these return values are not being used/asserted; they should be removed or used with assertions.

the changes entailed are minimum and could fit into the PR to ensure its correctness.

copyrighthero · 2026-03-24T16:07:56Z

components/ads-client/integration-tests/tests/http_cache.rs

+        .build()
+        .expect("cache build should succeed");
+
+    let url = Url::parse("https://ads.mozilla.org/v1/ads").unwrap();


It seems that we are testing against the production server here, should we be targeting stage instead? Or to target different envs based on the MozAdsEnvironment?

let url = Url::parse("https://ads.allizom.org/v1/ads").unwrap();

Same as ⬆️

I noticed that the github action does run these ignored tests, which is good but also poses a problem, every time there is a push, it will call the production server, which may not be desirable. Here are some options to consider:

To make the github action to run these tests to be manually controlled, remove the push/pull_request triggers and keep only workflow_dispatch.

To change the URL to stage, and devs could change it to prod when testing locally; or have it depend on the MozAdsEnvironment to toggle between automatically.

Or if we could confirm with MARS that these prod queries is fine and won't mess up with their stats or stuff.

.github/workflows/ads-client-tests.yaml

Almaju · 2026-03-24T18:13:52Z

Executed the unit tests locally and verified that they pass!

It seems that in the new unit tests, we:

* Stopped checking the `spoc.len() == count`, is this intentional?
  
  * Similarly, it seems that instead of testing 3 counts, we are only testing 1 in the new version.

* We stopped testing if the placement exists with the `placements.get(...).expect(...)`, I wonder if we still need to?

Good catch for the spoc len! I fixed it: 5786edf
For the placements.get(...).expect(...) I think this is covered by placements.contains_key(...)

copyrighthero · 2026-03-24T20:12:22Z

components/ads-client/integration-tests/tests/http_cache.rs

+            },
+        )
+        .unwrap();
+    assert!(matches!(o1.cache_outcome, CacheOutcome::MissStored));


Seems like we are already catching an error here!

copyrighthero · 2026-03-24T20:14:40Z

components/ads-client/integration-tests/tests/http_cache.rs

+    let o3 = cache
+        .send_with_policy(req, &RequestCachePolicy::default())
+        .unwrap();
+    assert!(matches!(o3.cache_outcome, CacheOutcome::MissStored));


Similar here! Both o1 and o3's cache_outcome was Hit instead of MissStored.

- Prune expired entries before every cache lookup in send_with_policy so expired entries are never served as hits - Change expiry comparison from strict < to <= so entries expire exactly at their TTL boundary, not one second after - Change send_with_policy return type to Vec<CacheOutcome> so both CleanupFailed and the primary outcome (Hit/MissStored/etc.) can be reported in the same call; HttpCacheSendResult is now a concrete type alias with no generic parameter - Remove delete_expired_entries from fetch_and_cache (now handled entirely in send_with_policy) - Add regression test: advancing the TestClock past TTL must yield MissStored, not Hit - Mark integration test as #[ignore] for manual runs; update CI workflow to use dtolnay/rust-toolchain and pass -- --ignored; consolidate stray ads-client-integration-tests.yml into ads-client-tests.yaml

copyrighthero

Alrighty!! Looking good! A few notes:

The use of prod url in testing may need some adjustment pending your decision.
I see that there are lingering matches! without assertions that could be remove or modified in a later commit, pending your decision; I would say that the three assertions helped us catch a bug!

copyrighthero · 2026-03-24T21:43:48Z

components/ads-client/integration-tests/tests/http_cache.rs

+        .build()
+        .expect("cache build should succeed");
+
+    let url = Url::parse("https://ads.mozilla.org/v1/ads").unwrap();


I noticed that the github action does run these ignored tests, which is good but also poses a problem, every time there is a push, it will call the production server, which may not be desirable. Here are some options to consider:

To make the github action to run these tests to be manually controlled, remove the push/pull_request triggers and keep only workflow_dispatch.

To change the URL to stage, and devs could change it to prod when testing locally; or have it depend on the MozAdsEnvironment to toggle between automatically.

Or if we could confirm with MARS that these prod queries is fine and won't mess up with their stats or stuff.

… stray matches! - Replace real HTTP calls in http_cache integration test with mockito, removing the viaduct-hyper dependency from that test path; add viaduct-dev and mockito to the integration-tests crate - Wrap 5 bare matches! calls in assert! in http_cache unit tests - Consolidate CI workflow: use dtolnay/rust-toolchain, pass -- --ignored to pick up ignored integration tests, remove stray workflow file

copyrighthero

Great job!! Tested the code locally to be working properly 🎊

Almaju mentioned this pull request Mar 18, 2026

feat(ads-client): add contract tests to validate types against MARS #7263

Closed

5 tasks

Almaju changed the title ~~feat(ads-client): add integration tests against MARS staging with GitHub CI workflow~~ test(ads-client): add integration tests against MARS staging with GitHub CI workflow Mar 18, 2026

Almaju marked this pull request as draft March 18, 2026 21:14

Almaju force-pushed the ads-client-integration-tests branch from d8d5a32 to 78066e5 Compare March 18, 2026 21:16

feat(ads-client): add integration tests against MARS staging

3be4824

Almaju force-pushed the ads-client-integration-tests branch from 78066e5 to 3be4824 Compare March 19, 2026 19:37

Almaju marked this pull request as ready for review March 19, 2026 19:58

Almaju added 4 commits March 19, 2026 16:50

docs(ads-client): update integration test instructions for new crate

79cfac6

refactor(ads-client): split integration tests into mars and http_cach…

3a61686

…e files

chore(ads-client): cargo fmt

3460898

copyrighthero reviewed Mar 24, 2026

View reviewed changes

fix(ads-client): fix spoc staging test assertion

5786edf

fix(ads-client): assert matches! in http_cache integration test

aa61c96

Almaju enabled auto-merge March 24, 2026 20:06

copyrighthero reviewed Mar 24, 2026

View reviewed changes

Almaju added 2 commits March 25, 2026 10:46

chore(ads-client): cargo fmt

43eb42b

copyrighthero approved these changes Mar 25, 2026

View reviewed changes

Almaju added this pull request to the merge queue Mar 25, 2026

Merged via the queue into mozilla:main with commit a80e67d Mar 25, 2026
15 checks passed

Almaju deleted the ads-client-integration-tests branch March 25, 2026 16:24

Conversation

Almaju commented Mar 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Pull Request checklist

Uh oh!

copyrighthero left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Almaju commented Mar 24, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

copyrighthero left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

copyrighthero left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Almaju commented Mar 18, 2026 •

edited

Loading