Add unit test coverage for LanguageNegotiationBrowser

Thank you @stefanos.petrakis!

1. Coding standards checking passes

2. +++ b/core/modules/language/tests/src/Unit/Plugin/LanguageNegotiation/LanguageNegotiationBrowserTest.php
   @@ -0,0 +1,127 @@
   +  protected function setUp() {
   

   See #3107732: Add return typehints to setUp/tearDown methods in concrete test classes, return type hinting is expected in 9.x now

3. +++ b/core/modules/language/tests/src/Unit/Plugin/LanguageNegotiation/LanguageNegotiationBrowserTest.php
   @@ -0,0 +1,127 @@
   +    $language_de = $this->createMock(LanguageInterface::class);
   +    $language_de->expects($this->any())
   

   Prefer using Prophecy for mocking, see https://www.drupal.org/docs/8/phpunit/using-prophecy, but it's ok to not use it. Non-mandatory.

4. +++ b/core/modules/language/tests/src/Unit/Plugin/LanguageNegotiation/LanguageNegotiationBrowserTest.php
   @@ -0,0 +1,127 @@
   +    // Create a language manager stub.
   ...
   +    // Create a user stub.
   

   Code here is self-explained. To me, comments are unnecessary, but it's ok to keep it if you like.

Switching to 9.1.x, could be backported to 8.x

Yep, better to fix injection instead of mocking Drupal::service()

Blocker is in

Looks it needs to re-upload patch 5, requeued

btw Looking on test coverage it looks better to create base test-class or trait with a method to create plugins via factory

+++ b/core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationBrowser.php
@@ -25,6 +28,30 @@ class LanguageNegotiationBrowser extends LanguageNegotiationMethodBase {
+  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {
+    return new static($container->get('page_cache_kill_switch'));

The plugin must get configuration into constructor, so test coverage should catch it

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

Scratched out the remaining tasks as they appear to be completed. The MR contains the test for getLangCode().

Coverage looks good

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Few small things but overall looks close.

Appears @jungle has one open thread if that can be answered.

Other then that looks good!

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Seeing some weirdness myself.

Opening MR 2900 I see there are 30 files changed.
Clicking file tab I see 2
If I go to https://git.drupalcode.org/project/drupal/-/merge_requests/2900.diff I see TONS of changes.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Seems to be good now but think this maybe should be postponed for #3130107: Extend unit test coverage for LanguageNegotiationContentEntity since the fixes intersect. This will have to be reviewed again when #3130107: Extend unit test coverage for LanguageNegotiationContentEntity lands

I was looking at this again and this appears to be a duplicate of #3130751: Fix LanguageNegotiationContentEntity::getLangcode() errors thrown, which was committed last month. Looking more closely at the test cases I think this is adding some, such as

    // Case 3: LanguageManager available, a request is given, but
    // HTTP_ACCEPT_LANGUAGE is not provided.

I think what needs to happen is to improve the testing in the existing \Drupal\Tests\language\Unit\Plugin\LanguageNegotiation\LanguageNegotiationContentEntityTest with any tests cases that are new here.