Closed Bug 1309481 Opened 8 years ago Closed 8 years ago

Remove leftover code specific to Logins from JSONFile.jsm and add tests

Categories

(Toolkit :: Form Manager, defect)

Product:
Toolkit
Component:
Form Manager
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: alchen, Assigned: lchang)

References

Details

(Whiteboard: [form autofill:M1])

Attachments

(1 file)

Bug 1309481 - Remove leftover code specific to Logins from JSONFile.jsm and add tests;
58 bytes, text/x-review-board-request
Paolo
: review+
Details 
In bug 1304322, Luke introduce a generic module called JSONFile.jsm.
However, I think the following is specific for password manager.
https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/JSONFile.jsm#168,220
In my opinion, we should move "nextId" related code into LoginStore.jsm.
Hi Paolo and Luke,
what do you think?
Flags: needinfo?(paolo.mozmail)
Flags: needinfo?(lchang)
Thanks for pointing it out. I agree with you and will take this bug once confirmed with Paolo.
Flags: needinfo?(lchang)
Yes, I recommend initializing to an empty object and setting the initial _nextId in the dataPostProcessor. Thanks Alphan for reporting this!

There are a few other things that should be done to complete the migration of the module to a general purpose one.

Firstly we need a specific test file, please "hg copy" the file "test_module_LoginStore.js" to "toolkit/modules/tests/xpcshell/test_JSONFile.js" and edit it to use generic data.

I'd also update the module to use "Task.async()", and I'd make the "save" method private. I don't see a use case for saving before the small delay, and it can race with the delayed save anyways. While the save is atomic, it's better not to take the risk! Example syntax:

  _save: Task.async(function* () {
    //...
  }),

This comment also needs updating:

https://dxr.mozilla.org/mozilla-central/source/toolkit/modules/JSONFile.jsm#158-161

It can be reworded and the reference to the original logins bug can be removed.

Also, the dataPostProcessor is now potentially fallible. The behavior in the code is correct - if the post processor fails, then both load() and ensureDataReady() will respectively reject or throw, and they would need to be called again before the data can be used. It's worth calling this out explicitly in the comments for load(), ensureDataReady(), and dataPostProcessor. Note that if the dataPostProcessor is infallible, then the two methods are infallible too.

If the data post processor fails, though, I'd reset the data to null so we don't keep unusable information in memory and avoid potential bugs with the callers ignoring the failure with the load. Actually, we can rename "data" to "_data" and make "data" a getter that throws if the data is not ready.

In addition to a generic test for dataPostProcessor, we must add a test for the case where dataPostProcessor throws.
Flags: needinfo?(paolo.mozmail)
Summary: Need to remove uncommon code in JSONFile.jsm → Remove leftover code specific to Logins from JSONFile.jsm and add tests
Hi Paolo,

Thanks for your recommendations. I have another bug in hand, but I'll take this bug if there's no hurry.
Thanks Luke, sounds good to me.
Assignee: nobody → lchang
Status: NEW → ASSIGNED
Hi Paolo,

I've updated "JSONFile.jsm" according to comment 3. Instead of "hg copy", I tried to write a new "test_JSONFile.js" and hope it would be more generic and clear for our purpose. Could you please take a look? Thanks a lot.
Comment on attachment 8801689 [details]
Bug 1309481 - Remove leftover code specific to Logins from JSONFile.jsm and add tests;

https://reviewboard.mozilla.org/r/86368/#review85050

Thanks, very good implementation!

There are a few edge cases to handle and test still.

::: toolkit/modules/JSONFile.jsm:114
(Diff revision 2)
> +  /**
> +   * Serializable object containing the data. This is populated directly with
>     * the data loaded from the file, and is saved without modifications.
>     *
>     * This contains one property for each list.
>     */
> -  data: null,
> +  _data: null,

Move this comment to the public getter, and I guess the last line does not apply.

Also worth repeating that "The raw data should be manipulated synchronously" like we say in the introduction.

::: toolkit/modules/JSONFile.jsm:134
(Diff revision 2)
> +  set data(newData) {
> +    if (!this.dataReady) {
> +      throw new Error("Data is not ready.");
> +    }
> +    this._data = newData;
> +  },

Hm, do we want to support a setter? If yes, it should probably call saveSoon automatically, and either way it should be documented. Also, setting this to |undefined| may cause issues with "JSON.stringify", and should be disallowed.

I see this setter is tested, but we should primarily test the case where we modify the data without using the setter and we have to call saveSoon necessarily.

::: toolkit/modules/JSONFile.jsm:141
(Diff revision 2)
>    /**
>     * Loads persistent data from the file to memory.
>     *
>     * @return {Promise}
>     * @resolves When the operation finished successfully.
> -   * @rejects JavaScript exception.
> +   * @rejects JavaScript exception or dataPostProcessor fails.
>     */

Worth mentioning that this never fails if there is no dataPostProcessor.

nit: @rejects JavaScript exception when dataPostProcessor fails.

::: toolkit/modules/JSONFile.jsm:191
(Diff revision 2)
>    /**
> -   * Loads persistent data from the file to memory, synchronously.
> +   * Loads persistent data from the file to memory, synchronously. Exception
> +   * will be thrown if dataPostProcessor fails.
>     */

Same, an exception can be thrown only if dataPostProcessor fails.

::: toolkit/modules/tests/xpcshell/test_JSONFile.js:74
(Diff revision 2)
> +
> +  return deferred.promise;
> +}
> +
> +
> +const TEST_FILE_PATH = getTempFile("test-json-file").path;

I think "let gTestFilePath" is clearer because this can vary between test runs, and it's a global shared between different test cases.

Anyways, while this is applied inconsistently and may vary from case to case, I generally prefer each task to be able to run independently, like "test_module_LoginStore.js" does.

This way, you can use the ".only" feature for debugging something specific locally:

add_task.only(function* test_ensure_data_ready() {
  //...
});

::: toolkit/modules/tests/xpcshell/test_JSONFile.js:78
(Diff revision 2)
> +
> +const TEST_FILE_PATH = getTempFile("test-json-file").path;
> +
> +const TEST_DATA = {
> +  number: 123,
> +  string: 'test',

nit: double quotes

::: toolkit/modules/tests/xpcshell/test_JSONFile.js:85
(Diff revision 2)
> +////////////////////////////////////////////////////////////////////////////////
> +//// Tests

A test similar to test_load_string_predefined is still needed to check that we don't change the format. For example, if someone thinks of compressing the data, this test failing would remind that we have to keep backwards compatibility.

Also test_load_string_malformed and test_load_string_malformed_sync must be tested.

::: toolkit/modules/tests/xpcshell/test_JSONFile.js:101
(Diff revision 2)
> +  storeForSave.saveSoon();
> +  yield sleep(100);

This is a recipe for intermittent failures... you should actually override the "_save" function during the test with one that calls the original implementation, then resolves a second Promise (accessible to the test) when the Promise from the original function resolves.

::: toolkit/modules/tests/xpcshell/test_JSONFile.js:110
(Diff revision 2)
> +  do_check_eq(storeForLoad.data.number, TEST_DATA.number);
> +  do_check_eq(storeForLoad.data.string, TEST_DATA.string);
> +  do_check_eq(JSON.stringify(storeForLoad.data.object),
> +    JSON.stringify(TEST_DATA.object));

You can use "Assert.deepEqual" here and below.

::: toolkit/modules/tests/xpcshell/test_JSONFile.js:161
(Diff revision 2)
> +  yield store.load().catch(() => {
> +    fail = true;
> +  });

You can use "Assert.rejects", and "Assert.throws" below, also to check that the error message is propagated.

::: toolkit/modules/tests/xpcshell/test_JSONFile.js:169
(Diff revision 2)
> +
> +  do_check_true(fail);
> +  do_check_false(store.dataReady);
> +});
> +
> +add_task(function* test_ensure_data_ready_with_data_post_processor_fails()

nit: we generally don't alter the names of functions under test, for example test_ensureDataReady_with_dataPostProcessor_fails, so their test names show up when doing a string search.
Attachment #8801689 - Flags: review?(paolo.mozmail)
Comment on attachment 8801689 [details]
Bug 1309481 - Remove leftover code specific to Logins from JSONFile.jsm and add tests;

https://reviewboard.mozilla.org/r/86368/#review85050

> Hm, do we want to support a setter? If yes, it should probably call saveSoon automatically, and either way it should be documented. Also, setting this to |undefined| may cause issues with "JSON.stringify", and should be disallowed.
> 
> I see this setter is tested, but we should primarily test the case where we modify the data without using the setter and we have to call saveSoon necessarily.

I'm not sure if there are use cases, either. Since Form Autofill doesn't need the setter by now, I'll remove it until we find the use case.

> I think "let gTestFilePath" is clearer because this can vary between test runs, and it's a global shared between different test cases.
> 
> Anyways, while this is applied inconsistently and may vary from case to case, I generally prefer each task to be able to run independently, like "test_module_LoginStore.js" does.
> 
> This way, you can use the ".only" feature for debugging something specific locally:
> 
> add_task.only(function* test_ensure_data_ready() {
>   //...
> });

You're right. Thanks for pointing it out.

> This is a recipe for intermittent failures... you should actually override the "_save" function during the test with one that calls the original implementation, then resolves a second Promise (accessible to the test) when the Promise from the original function resolves.

I also struggled to use it. Promise is better indeed. Thanks.
Whiteboard: [form autofill:M1]
Comment on attachment 8801689 [details]
Bug 1309481 - Remove leftover code specific to Logins from JSONFile.jsm and add tests;

https://reviewboard.mozilla.org/r/86368/#review85526

Wonderful! Thanks for the excellent code.

::: toolkit/modules/tests/xpcshell/test_JSONFile.js:143
(Diff revisions 2 - 4)
>      },
>    });
>  
> -  yield store.load();
> +  yield storeForLoad.load();
>  
> -  do_check_eq(store._data.test, random);
> +  do_check_eq(storeForLoad._data.test, random);

Should use the "data" getter here.

::: toolkit/modules/tests/xpcshell/test_JSONFile.js:169
(Diff revisions 2 - 4)
>        throw new Error("dataPostProcessor fails.");
>      },
>    });
> -  let fail = false;
>  
> -  try {
> +  Assert.throws(store.ensureDataReady.bind(store), /dataPostProcessor fails\./);

nit: () => store.ensureDayaReady()
Attachment #8801689 - Flags: review?(paolo.mozmail) → review+
Blocks: 1304322, 1016733
Hi Paolo,

Patch has been updated and is running on try server. Thanks for your review.
Tests passed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=77b8a1cc1951
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/170c0f7ef959
Remove leftover code specific to Logins from JSONFile.jsm and add tests; r=Paolo
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/170c0f7ef959
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: