1022181 - Mobile ID Tests

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Comment 1

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 1: DOM API tests

Comment 2

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 2: Mobile Identity service tests

Sorry for the late here, I am finding several issues with mochitests and xpcshell tests in b2g :(...

Comment 3

[Lawrence Mandel [:lmandel] (use needinfo)]

Fernando - Can you get tests written to support bug 1022193 that can be uplifted at the same time?

Comment 4

[Fernando Jiménez Moreno [:ferjm]]

Yes, I'll try to have them asap.

Comment 5

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 2: Mobile Identity service tests

Comment 6

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 2: Mobile Identity service tests

Comment 7

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 1: DOM tests

Comment 8

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 2: Mobile ID Service tests

Comment 9

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

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?

Comment 10

[Fernando Jiménez Moreno [:ferjm]]

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!

Comment 11

[Fernando Jiménez Moreno [:ferjm]]

Thanks Jed and Jonas!

https://hg.mozilla.org/integration/b2g-inbound/rev/2edd10045963
https://hg.mozilla.org/integration/b2g-inbound/rev/05e7b540b5c1

Comment 12

[Fernando Jiménez Moreno [:ferjm]]

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

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2edd10045963
https://hg.mozilla.org/mozilla-central/rev/05e7b540b5c1

Comment 14

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8451621 [details] [diff] [review]
Part 1: DOM tests

Aurora+

Comment 15

[Fernando Jiménez Moreno [:ferjm]]

Thanks Lawrence.

Before uplifting this patch, bug 1022193 and bug 1026999 needs to land in Aurora.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/620e68b0053d
https://hg.mozilla.org/releases/mozilla-aurora/rev/bcd461c3bb21