Skip to content

Bug 1714578 - Pings testing api docs#1691

Merged
Dexterp37 merged 5 commits intomozilla:mainfrom
Dexterp37:pings_testing_api_docs
Jun 17, 2021
Merged

Bug 1714578 - Pings testing api docs#1691
Dexterp37 merged 5 commits intomozilla:mainfrom
Dexterp37:pings_testing_api_docs

Conversation

@Dexterp37
Copy link
Copy Markdown
Contributor

@Dexterp37 Dexterp37 commented Jun 14, 2021

[docs only]

This includes documentation for the testing API
which was missing.
@Dexterp37 Dexterp37 requested a review from brizental June 14, 2021 16:30
@Dexterp37 Dexterp37 self-assigned this Jun 14, 2021
@auto-assign auto-assign Bot requested a review from badboy June 14, 2021 16:30
@Dexterp37 Dexterp37 removed the request for review from badboy June 14, 2021 16:30
Copy link
Copy Markdown
Contributor

@brizental brizental left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed the submit section is lacking Java examples, would you mind filing a follow-up for that or just doing it in this PR?

On the examples you create this validatorRun boolean that is not really necessary it seems, or am I missing something? If not, can we remove that from the examples so as to not add more to it than necessary?

Comment thread docs/user/user/pings/testing-custom-pings.md Outdated
Comment thread docs/user/user/pings/testing-custom-pings.md
Comment thread docs/user/user/pings/testing-custom-pings.md Outdated
Comment thread docs/user/reference/pings/index.md
Comment thread docs/user/reference/pings/index.md
Comment thread docs/user/reference/pings/index.md Outdated
@Dexterp37 Dexterp37 requested a review from brizental June 15, 2021 12:17
@Dexterp37
Copy link
Copy Markdown
Contributor Author

On the examples you create this validatorRun boolean that is not really necessary it seems, or am I missing something? If not, can we remove that from the examples so as to not add more to it than necessary?

The boolean is used to make sure that the validator function actually runs. We have no other way to be completely sure of that in our APIs. We can't distinguish between "the callback run and detected no error" with the "callback did not run at all" (we do that in our tests, too)

@brizental
Copy link
Copy Markdown
Contributor

The boolean is used to make sure that the validator function actually runs. We have no other way to be completely sure of that in our APIs. We can't distinguish between "the callback run and detected no error" with the "callback did not run at all" (we do that in our tests, too)

In this case, why does the Rust example not have that?

Copy link
Copy Markdown
Contributor

@brizental brizental left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r+wc

Comment thread docs/user/user/pings/testing-custom-pings.md
{{#include ../../../shared/tab_footer.md}}
1. Triggering the code path that accumulates/records the data.
2. Defining a callback validation function using the [ping testing API](../../reference/pings/index.md#testbeforenextsubmit).
3. Finally triggering the code path that submits the custom ping or submitting the ping using the [`submit` API](../../reference/pings/index.md#submit).
Copy link
Copy Markdown
Contributor

@brizental brizental Jun 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO, we could go further and maybe go through a real world case of ping testing in this page. It is kinda dry at the moment i.e. does not add too much to the docs. Good enough for this PR, but would you mind filing a follow-up bug?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Member

@badboy badboy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The boolean is used to make sure that the validator function actually runs. We have no other way to be completely sure of that in our APIs. We can't distinguish between "the callback run and detected no error" with the "callback did not run at all" (we do that in our tests, too)

In this case, why does the Rust example not have that?

Probably because it's so much more annoying to do in Rust:

let callback_was_called = Arc::new(AtomicBool::new(false));
let callback_was_called_2 = Arc::clone(&callback_was_called);

ping.test_before_next_submit(move |reason| {
    assert_eq!(None, reason);
    assert_eq!(1, metric.test_get_value(None).unwrap());
    callback_was_called_2.store(true, Ordering::SeqCst);
});

ping.submit(None);
assert!(callback_was_called.load(Ordering::SeqCst));

I'd say if you directly call ping.submit() in your test there's no need for callback_was_called (because users should trust us that we call it from submit).
It's only when you invoke submit behind the scenes and custom logic it might be a good idea I guess.

Comment thread docs/user/reference/pings/index.md Outdated
@Dexterp37 Dexterp37 enabled auto-merge June 17, 2021 08:01
@Dexterp37 Dexterp37 merged commit 4102e60 into mozilla:main Jun 17, 2021
@Dexterp37 Dexterp37 deleted the pings_testing_api_docs branch June 17, 2021 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants