Closed Bug 1022181 Opened 5 years ago Closed 5 years ago

Mobile ID Tests

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33
Tracking Status
firefox31 --- wontfix
firefox32 --- fixed
firefox33 --- fixed
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: ferjm, Assigned: ferjm)

References

Details

(Whiteboard: ft:loop)

Attachments

(2 files, 4 obsolete files)

No description provided.
Assignee: nobody → ferjmoreno
Whiteboard: ft:loop
Attached patch Part 1: DOM API tests (obsolete) — Splinter Review
Sorry for the late here, I am finding several issues with mochitests and xpcshell tests in b2g :(...
Fernando - Can you get tests written to support bug 1022193 that can be uplifted at the same time?
Flags: needinfo?(ferjmoreno)
Yes, I'll try to have them asap.
Flags: needinfo?(ferjmoreno)
Blocks: 1026999
Attachment #8445845 - Attachment is obsolete: true
Attachment #8451046 - Attachment is obsolete: true
Attachment #8451494 - Flags: review?(jed+bmo)
Attachment #8445844 - Attachment is obsolete: true
Attachment #8451621 - Flags: review?(jonas)
Attachment #8451494 - Attachment is obsolete: true
Attachment #8451494 - Flags: review?(jed+bmo)
Attachment #8451622 - Flags: review?(jed+bmo)
Comment on attachment 8451622 [details] [diff] [review]
Part 2: Mobile ID Service tests

Review of attachment 8451622 [details] [diff] [review]:
-----------------------------------------------------------------

I must confess I have not scrutinized the sequence of spied calls in minute detail.  To do so, I will need to get to know the state machine better.  If you want extra scrutiny of those subtle sequences, please let me know and I will dig deeper.

Always nice to see a substantial test suite such as this.  With a few nits and questions addressed, r=me.

Thanks, Fernando.

::: services/mobileid/tests/xpcshell/test_mobileid_manager.js
@@ +40,5 @@
> +const iccId = "aIccId";
> +const mnc = "aMnc";
> +const mcc = 214;
> +const operator = "aOperator";
> +const certificate = "eyJhbGciOiJEUzI1NiJ9.eyJsYXN0QXV0aEF0IjoxNDA0NDY5NzkyODc3LCJ2ZXJpZmllZE1TSVNETiI6IiszMTYxNzgxNTc1OCIsInB1YmxpYy1rZXkiOnsiYWxnb3JpdGhtIjoiRFMiLCJ5IjoiNGE5YzkzNDY3MWZhNzQ3YmM2ZjMyNjE0YTg1MzUyZjY5NDcwMDdhNTRkMDAxMDY4OWU5ZjJjZjc0ZGUwYTEwZTRlYjlmNDk1ZGFmZTA0NGVjZmVlNDlkN2YwOGU4ODQyMDJiOTE5OGRhNWZhZWE5MGUzZjRmNzE1YzZjNGY4Yjc3MGYxZTU4YWZhNDM0NzVhYmFiN2VlZGE1MmUyNjk2YzFmNTljNzMzYjFlYzBhNGNkOTM1YWIxYzkyNzAxYjNiYTA5ZDRhM2E2MzNjNTJmZjE2NGYxMWY3OTg1YzlmZjY3ZThmZDFlYzA2NDU3MTdkMjBiNDE4YmM5M2YzYzVkNCIsInAiOiJmZjYwMDQ4M2RiNmFiZmM1YjQ1ZWFiNzg1OTRiMzUzM2Q1NTBkOWYxYmYyYTk5MmE3YThkYWE2ZGMzNGY4MDQ1YWQ0ZTZlMGM0MjlkMzM0ZWVlYWFlZmQ3ZTIzZDQ4MTBiZTAwZTRjYzE0OTJjYmEzMjViYTgxZmYyZDVhNWIzMDVhOGQxN2ViM2JmNGEwNmEzNDlkMzkyZTAwZDMyOTc0NGE1MTc5MzgwMzQ0ZTgyYTE4YzQ3OTMzNDM4Zjg5MWUyMmFlZWY4MTJkNjljOGY3NWUzMjZjYjcwZWEwMDBjM2Y3NzZkZmRiZDYwNDYzOGMyZWY3MTdmYzI2ZDAyZTE3IiwicSI6ImUyMWUwNGY5MTFkMWVkNzk5MTAwOGVjYWFiM2JmNzc1OTg0MzA5YzMiLCJnIjoiYzUyYTRhMGZmM2I3ZTYxZmRmMTg2N2NlODQxMzgzNjlhNjE1NGY0YWZhOTI5NjZlM2M4MjdlMjVjZmE2Y2Y1MDhiOTBlNWRlNDE5ZTEzMzdlMDdhMmU5ZTJhM2NkNWRlYTcwNGQxNzVmOGViZjZhZjM5N2Q2OWUxMTBiOTZhZmIxN2M3YTAzMjU5MzI5ZTQ4MjliMGQwM2JiYzc4OTZiMTViNGFkZTUzZTEzMDg1OGNjMzRkOTYyNjlhYTg5MDQxZjQwOTEzNmM3MjQyYTM4ODk1YzlkNWJjY2FkNGYzODlhZjFkN2E0YmQxMzk4YmQwNzJkZmZhODk2MjMzMzk3YSJ9LCJwcmluY2lwYWwiOiIwMzgxOTgyYS0xZTgzLTI1NjYtNjgzZS05MDRmNDA0NGM1MGRAbXNpc2RuLWRldi5zdGFnZS5tb3phd3MubmV0IiwiaWF0IjoxNDA0NDY5NzgyODc3LCJleHAiOjE0MDQ0OTEzOTI4NzcsImlzcyI6Im1zaXNkbi1kZXYuc3RhZ2UubW96YXdzLm5ldCJ9."

I don't have particularly strong feelings about it, but it would make it slightly easier to read the tests below if these names were capitalised (as you do for DEBUG and GET_ASSERTION*).  Alternately, stick a 'k' at the beginning of each, as you do below with kMobileIdentityClient etc.?

@@ +145,5 @@
> +  },
> +
> +  startFlow: function() {
> +    this._spy("startFlow", arguments);
> +    Promise.defer();

Pardon my ignorance and please enlighten me if I'm missing a subtle use of Promise.defer(), but do these calls do anything?

@@ +196,5 @@
> +
> +  getByOrigin: function() {
> +    this._spy("getByOrigin", arguments);
> +    Promise.defer();
> +    let result = this._options.getByOriginResult || this._getByOriginResult;

You don't use 'result'

@@ +341,5 @@
> +}) {
> +  let XULAppInfoFactory = {
> +    createInstance: function (outer, iid) {
> +      if (outer != null)
> +        throw Cr.NS_ERROR_NO_AGGREGATION;

nit: squiggly brackets for 'if' consequent

@@ +357,5 @@
> +
> +// Unregister mocks and restore original code.
> +do_register_cleanup(function() {
> +  MobileIdentityManager.credStore = kMobileIdentityCredStore;
> +  MobileIdentityManager.client = kMobileIdentityClient;

Your tests also modify the iccInfo and ui properties.  I believe do_register_cleanup() at the top level is only run after the last test in the file has been executed.  You can define the cleanup function at the top level and invoke do_register_cleanup(cleanup) at the beginning of each test?
Attachment #8451622 - Flags: review?(jed+bmo) → review+
Comment on attachment 8451621 [details] [diff] [review]
Part 1: DOM tests

I'm asking also Olli for r? because this is blocking the uplift of a few patches. Thanks!
Attachment #8451621 - Flags: review?(bugs)
Comment on attachment 8451621 [details] [diff] [review]
Part 1: DOM tests

Approval Request Comment
[Feature/regressing bug #]: Mobile Identity
[User impact if declined]: Potential regressions
[Describe test coverage new/current, TBPL]: These are the tests :)
[Risks and why]: None, just tests
[String/UUID change made/needed]: None
Attachment #8451621 - Flags: review?(bugs) → approval-mozilla-aurora?
Attachment #8451622 - Flags: approval-mozilla-aurora?
No longer blocks: 1022193
No longer blocks: 1026999
Depends on: 1026999
https://hg.mozilla.org/mozilla-central/rev/2edd10045963
https://hg.mozilla.org/mozilla-central/rev/05e7b540b5c1
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Comment on attachment 8451621 [details] [diff] [review]
Part 1: DOM tests

Aurora+
Attachment #8451621 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8451622 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Thanks Lawrence.

Before uplifting this patch, bug 1022193 and bug 1026999 needs to land in Aurora.
You need to log in before you can comment on or make changes to this bug.