Removal of browser-storage-memory

[4shutosh]

Pull Request checklist

Fixes #11173

• Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
• Tests: This PR includes thorough tests or an explanation of why it does not
• Changelog: This PR includes a changelog entry or does not need one
• Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

• Milestone: Make sure issues closed by this pull request are added to the milestone of the version currently in development.
• Breaking Changes: If this is a breaking change, please push a draft PR on Reference Browser to address the breaking issues.

[4shutosh]

I was not sure about the tests using the InMemoryHistoryStorage so I've created a class for each of them.
Please let me know if that's wrong and what other changes are required.

[pocmo]

Thank you for your PR! The removal looks good.

I'll defer to @grigoryk regarding the unit tests. The duplication of the in-memory implementation is a bit unfortunate.

[grigoryk]

The deletion looks good, thanks!

As for the tests, I don't think we need to duplicate these fake implementations.

I think if we were writing these tests from scratch now, we'd just use a mock of the storage, and configure that mock to return the data necessary for the specific tests in question. This will involve updating all of these tests - whenever we add something to the storage, provide the data via the mock instead.

This matches the "test pyramid" kind of approach - we're not testing the storage here, just how we interact with it.

Another option which is also fine and much easier is to update these tests to use PlacesHistoryStorage as a quick way to move forward here. The main downside is that it'll slow down the tests a bit, I think - but probably not by too much.

You'd need to add a testImplementation of browser-storage-sync and testImplementation Dependencies.mozilla_full_megazord_forUnitTests to the gradle files for the affected modules.
Then, you can just just swap out the storage instances in tests with something like val historyStore = PlacesHistoryStorage(testContext).

For an example of this, tests in support-migration use the browser-storage-sync storage layer.

[4shutosh]

Thank you for the detailed explanation @grigoryk.

I've done the changes suggested by you, please take a look and let me know if any other changes are required.

[pocmo]

There are 11 failed tests in feature-awesomebar and 1 failed test in feature-toolbar. Does this reproduce locally?

[4shutosh]

Yes, they failed locally as well.

the log is similar to the online one

[4shutosh]

Now the tests are getting instrumented correctly but are failing logically due to the usage of PlacesHistoryStorage.

I'll have to dig deep to get an understanding of how they are working. Any other option to work with :)?

[grigoryk]

Now the tests are getting instrumented correctly but are failing logically due to the usage of PlacesHistoryStorage.

I'll have to dig deep to get an understanding of how they are working. Any other option to work with :)?

Thanks for updating the PR! It seems like assumptions tests were making about behaviour of the old in-memory store don't apply to the real one. I think it's a good thing actually, maybe we'll uncover some real bugs here!

I think the way forward is to work through each test failure one-by-one, seeing what's going wrong, fixing the tests and maybe fixing underlying issue if there is one. That's likely quite a bit past the good-first-issue level of involvement at this point, so I'm happy to work through it locally. I'll start with the awesomebar failures, feel free to push more commits to the PR as well.

[4shutosh]

Okay. I'll try to work with the toolbar failures then.

[grigoryk]

@4shutosh I've updated tests in feature-awesomebar, let's see if they pass now. Take a look - I've removed the browser-storage-sync dependency entirely, switching tests to use mocked storage.

It'd be great if you could do the same for feature-toolbar while we're at it, switch it to use mocked storage (e.g. following the pattern in my commit), and remove the unnecessary testImplementation dependencies.

[4shutosh]

Sure. On it.

[4shutosh]

I've done the changes but encountered this failure

[grigoryk]

I've done the changes but encountered this failure

Yeah, that's expected. You switched to use a mock, but kept the old recordVisit calls - they don't actually do anything anymore, and so the test isn't passing now.

You'll need to replace bits where we insert stuff into the storage with corresponding mock statements.

For example:

runBlocking {
            history.recordVisit("https://www.mozilla.org", PageVisit(VisitType.TYPED, RedirectSource.NOT_A_SOURCE))
}

should be replaced by something like:

doReturn(listOf(HistoryAutocompleteResult(...)).`when`(history).getAutocompleteSuggestion(...)

Before, we'd record a record into the real storage, and it'll just work (since the real storage will also be queried by the toolbar feature). Now, we don't have a real storage to record records into, so we need to fake the "read" part of the storage instead to achieve the same effect for the test.

See what ToolbarAutocompleteFeature will do. It has something like

val historyResults = historyProviders.asSequence()
                .mapNotNull { provider -> provider.getAutocompleteSuggestion(query)?.into() }

... which tells us that getAutocompleteSuggestion is the storage call we need to mock. That's the "read from storage" bit we need to fake in the test.

See 3af92af for mocking examples.

[4shutosh]

Thank you for the beautiful explanation @grigoryk.

The tests pass now.

I feel I did not try enough to look for that, that was easy enough to be done by myself.

Are there any other things that need to be done for this PR? If not please assign me another issue or task as you know better what next should I get going with on this project.

[grigoryk]

Just a few small things and this should be good to land!

[grigoryk]

Thank you for the beautiful explanation @grigoryk.

The tests pass now.

I feel I did not try enough to look for that, that was easy enough to be done by myself.

Are there any other things that need to be done for this PR? If not please assign me another issue or task as you know better what next should I get going with on this project.

No worries, and thanks for contributing! Looking at the list of issues, we're a bit short right now on those tagged as help wanted, but that doesn't mean there aren't good projects/issues to work on. I'll look around and will post some links here tomorrow.

[grigoryk]

✨

[4shutosh]

Thank you for the help 🙌🏻

I'll look around and will post some links here tomorrow.

Yes please do.