Problem/Motivation

#3263654: Move all HAL tests to the module in preparation of removal in D10 moved all HAL tests out of the module, but some of them were generic tests using HAL rather than tests of HAL, so we need to move them back and refactor to use regular json to avoid losing test coverage.

RestRegisterUserTests from user module is one.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#12 interdiff_4-12.txt464 bytesmurilohp
#12 3268105-12.patch8.29 KBmurilohp
#5 diff_RestRegisterUserTest_UserRegistrationRestTest.txt2.49 KBmurilohp
#4 3268105-4.patch8.29 KBmurilohp
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch created an issue. See original summary.

catch credited TR.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

Crediting TR who spotted this.

murilohp’s picture

murilohp CreditAttribution: murilohp at CI&T commented
Status: Active » Needs review
FileSize
3268105-4.patch8.29 KB

This issue is blocking #3055807: User created via /user/register?_format=json get blocked, so I just rollback RestRegisterUserTest into the user module and removed the hal integration, the filename is now UserRegistrationRestTest, the idea of a new name is to follow the user's tests pattern.

murilohp’s picture

murilohp CreditAttribution: murilohp at CI&T commented
FileSize
diff_RestRegisterUserTest_UserRegistrationRestTest.txt2.49 KB

Just to help you out here with the review, here's a diff between the old test(core/modules/hal/tests/src/Functional/user/RestRegisterUserTest.php) and the new one(core/modules/user/tests/src/Functional/UserRegistrationRestTest.php)

TR’s picture

TR CreditAttribution: TR commented
Related issues: +#3268078: DBLogResource is no longer being tested
bbrala’s picture

bbrala
Location Netherlands
CreditAttribution: bbrala at SWIS commented
Status: Needs review » Reviewed & tested by the community

Submitted to early.

bbrala’s picture

bbrala
Location Netherlands
CreditAttribution: bbrala at SWIS commented
Status: Reviewed & tested by the community » Needs review
bbrala’s picture

bbrala
Location Netherlands
CreditAttribution: bbrala at SWIS commented
Status: Needs review » Reviewed & tested by the community

I've gone through the changes and they are looking good. Ran test locally and they passed also. I did this for d9 and d10.

The testfailure on 10.0.x is not related to this change so setting RTBC while we wait for green tests.

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje as a volunteer commented
Status: Reviewed & tested by the community » Needs work

Sorry to be the nit-picker, but I have 1 _very_ minor comment:

  1. diff --git a/core/modules/user/tests/src/Functional/UserRegistrationRestTest.php b/core/modules/user/tests/src/Functional/UserRegistrationRestTest.php
new file mode 100644
index 0000000000..8617be2ff7
--- /dev/null
+++ b/core/modules/user/tests/src/Functional/UserRegistrationRestTest.php

+/**
+ * Tests registration of user using rest.

    Can we have "rest" in CAPS here, to fit in with the rest *baduum-tish* of Core?

bbrala’s picture

bbrala
Location Netherlands
CreditAttribution: bbrala at SWIS commented

Not entirely true, but seems to be the prevalent version by far. :P (only found one lowercase in a quick search in CsrfRequestHeaderAccessCheck.php)

murilohp’s picture

murilohp CreditAttribution: murilohp at CI&T commented
FileSize
3268105-12.patch8.29 KB
interdiff_4-12.txt464 bytes

Addressing #10 here.

murilohp’s picture

murilohp CreditAttribution: murilohp at CI&T commented
Status: Needs work » Needs review
bbrala’s picture

bbrala
Location Netherlands
CreditAttribution: bbrala at SWIS commented
Status: Needs review » Reviewed & tested by the community

Well done, thanks.

Spokje’s picture

Spokje
Primary language Dutch
CreditAttribution: Spokje at Chromatic commented

Thanks @murilohp!

RTBC if TestBot agrees

INSTA-EDIT: ^What @bbrala said...

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Fixed

Committed f73d11a and pushed to 10.0.x. Thanks!

Backported to 9.4.x so the test is in 9 and 10.

  • alexpott committed 426e517 on 10.0.x 
    Issue #3268105 by murilohp, bbrala, catch, TR, Spokje: Bring back...

  • alexpott committed 56a355e on 9.4.x 
    Issue #3268105 by murilohp, bbrala, catch, TR, Spokje: Bring back...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.