It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

I'm not sure where to look for writing a test or even how to, so if the PR reviewer could guide me that would help a lot.

The change itself LGTM. Is there a way to write a test that would make sure we don't break it in the future?

Maybe something like this? https://github.com/flutter/flutter/blob/master/dev/bots/service_worker_test.dart

@yjbanov i think a test that checks if the '/' is present at the beginning of the file names is sufficient ?

Of course, since we don't have a test environment that replicates Github pages or similar, there's no guarantee that we won't break this again in the future. We won't break it on purpose though.

(Just checked in the original issue, and the headers are not overriding service worker scopes, odd)

For some reason I thought index.html was not served from the service worker in offline mode (or any mode), but maybe I'm wrong.

Well, our service worker does fetch it, but maybe that's unnecessary? We don't really support offline mode very easily right now, so all this may be a little moot.

(I think this can be merged, but it'll need revisiting when solving the issue you linked for sure @yjbanov)

@nicolasvac looks like there's a type error that the Linux web_long_running_tests_1_5 CI step is catching:

Unhandled exception:
type '_InternalLinkedHashMap<dynamic, dynamic>' is not a subtype of type 'Map<Object, Object>' of 'other'
#0      MapMixin.addAll (dart:collection/maps.dart)
#1      addStateInfo (package:matcher/src/util.dart:29:14)
#2      _DeepMatcher.matches (package:matcher/src/equals_matcher.dart:261:5)
#3      expect (file:///b/s/w/ir/x/w/flutter/dev/bots/service_worker_test.dart:61:15)
#4      runWebServiceWorkerTest.expectRequestCounts (file:///b/s/w/ir/x/w/flutter/dev/bots/service_worker_test.dart:76:5)
#5      runWebServiceWorkerTest (file:///b/s/w/ir/x/w/flutter/dev/bots/service_worker_test.dart:137:5)
<asynchronous suspension>
#6      _runShardRunnerIndexOfTotalSubshard (file:///b/s/w/ir/x/w/flutter/dev/bots/test.dart:1931:5)
<asynchronous suspension>
#7      _runWebLongRunningTests (file:///b/s/w/ir/x/w/flutter/dev/bots/test.dart:1115:3)
<asynchronous suspension>
#8      _runFromList (file:///b/s/w/ir/x/w/flutter/dev/bots/test.dart:1973:5)
<asynchronous suspension>
#9      main (file:///b/s/w/ir/x/w/flutter/dev/bots/test.dart:200:5)
<asynchronous suspension>

https://logs.chromium.org/logs/flutter/buildbucket/cr-buildbucket/8824686530296893649/+/u/run_test.dart_for_web_long_running_tests_shard_and_subshard_1_5/test_stderr

@christopherfujino how is this affected by removing the first element of the list, considering it's still a list ?

@christopherfujino how is this affected by removing the first element of the list, considering it's still a list ?

I didn't actually look at your PR before. Now that I did...I have no idea

I scheduled a re-run of Linux web_long_running_tests_1_5. If that also fails, I would suggest rebasing upstream, in case this was something that has already been fixed in master.

@christopherfujino merged commits

I'm stumped on this failure, gonna ask about it on the tests channel of our discord server.

Ok, narrowed it down to an bug in the test file: #99522. I will submit a patch to fix.

Fix is in #99524

Fix is in #99524

Fix has been merged, @nicolasvac please rebase upstream. The test will still fail because you need to update an expectation, but the failure should tell you how to fix it.

@christopherfujino i've remerged everything and waited for the tests to run,
[CHROME STDERR]:Fontconfig warning: "/etc/fonts/fonts.conf", line 100: unknown element "blank" [CHROME STDERR]:

Still this is not related to this merge request. I hope this can be merged now as it has been over two months for "outside" problems

@christopherfujino i've remerged everything and waited for the tests to run, [CHROME STDERR]:Fontconfig warning: "/etc/fonts/fonts.conf", line 100: unknown element "blank" [CHROME STDERR]:

Still this is not related to this merge request. I hope this can be merged now as it has been over two months for "outside" problems

Per the Flutter team's Tree Hygiene policy this change cannot be merged with failing tests.

The customer_testing-linux test will be fixed by rebasing (not merging) upstream. As for the web test infrastructure failure, I am not familiar with that. I would recommend joining the discord #hackers-infra channel and following up there (alternatively you can file an issue for it).

The failures seem unrelated to this PR and are probably caused by the branch being stale.

@nicolasvac could you please try rebasing your branch on top of flutter:master to see if that fixes the failures? You may need to remove the merge commits before attempting to rebase.