Problem/Motivation

Create `TimezoneFieldTest` class, to contain test methods that can be applied to any widget/formatter datetime/date_range in order to test timezone handling. This way if we add some new timezone logic, we only have to touch a couple of places in the test and it magically gets tested on all widgets/formatters.

Steps to reproduce

N/A

Proposed resolution

- Create TimezoneFieldTest
- Add test methods for datetime and daterange for timezone handling.

Remaining tasks

- Add tests as above
- Extract similar logic in Trait if required.
- Create MR or Patch.

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

Added TimezoneFieldTest

CommentFileSizeAuthor
#18 3185728-nr-bot.txt149 bytesneeds-review-queue-bot
#12 3185728-12.git-fork-screenshot.png42.73 KBdww
#11 interdiff_4-11.txt4.07 KBvinai_katiyar
#11 3185728-11.patch4.52 KBvinai_katiyar
#4 3185728-04.patch3.72 KBmpdonadio

Issue fork drupal-3185728

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

amjad1233 created an issue. See original summary.

dww’s picture

dww
we/he/they
commented
Parent issue: #2632040: [PP-1] Add ability to select a timezone for datetime field » #3185747: [META] Add timezone support to core date fields

Thanks for opening this! Re-parenting this to the new working plan meta, not the overloaded original parent with 290 comments. ;)

dww’s picture

dww
we/he/they
commented
Issue tags: +Needs tests

Also moving 'Needs tests' tag from the grandparent issue to here.

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
commented
Status: Active » Needs review
StatusFileSize
new 3185728-04.patch3.72 KB

OK, here is a start. It moved the time zones use in "the loop" into a helper class so that they can be used in Kernel as well as Functional tests.

Status: Needs review » Needs work

The last submitted patch, 4: 3185728-04.patch, failed testing. View results

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
commented

Blork. Stared at this fail for a while and remembered the autoloader doesn't really work in the tests/src directory, except for traits. And anything ending in Test is expected to an actual test class. As someone responsible for a fair bit of the mess in DTFT and DRFT, I suggest the following:

- Refactor DateTestBase::createField() to accept widget and formatter parameters.
- Add helpers / asserts to DateTestBase for TZ stuff
- Refactor the test methods to not reuse the same field instance, but have a test method per widget/formatter/setting and create the field in the test method and not the setUp which should result in identifying common code that can be moved onto DateTestBase

I don't want to derail this, but those two tests are a total mess (there is an old meta somewhere about needing to refactor them). I think this will go a long way into landing the parent.

?

dww’s picture

dww
we/he/they
commented

Yay, great to see you here @mpdonadio!

I opened an issue fork for this and pushed #4:

https://git.drupalcode.org/issue/drupal-3185728/-/tree/3185728-create-ti...

Shall we try that instead of patches for this? It'll only start posting comments here if we open an MR, but that seems premature. ;)

Re: #6. I haven't looked closely at those tests, so I'm not sure what to say. But I trust your judgement, that plan sounds both reasonable and do-able, and improving tests that "are a total mess" sounds good to me! If we should postpone this on that refactoring meta and start there, so be it. We can add it to #3185747: [META] Add timezone support to core date fields if needed. Or we can do it here and expand the scope of this. I almost never get the scoping decisions right. ;)

I don't see the meta here:
https://www.drupal.org/project/issues/drupal?text=DateTestBase&status=All

Nor here:
https://www.drupal.org/project/issues/search/drupal?text=refactor+date+t...

Got a link?

Thanks!
-Derek

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented 
+++ b/core/modules/datetime/tests/src/TimezoneFieldTestHelper.php
@@ -0,0 +1,32 @@
+class TimezoneFieldTestHelper {

Why can't this be a trait?

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
commented

@dww, awesome. I rewound my work and am going to start on the refactors there.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

vinai_katiyar’s picture

vinai_katiyar commented
Status: Needs work » Needs review
StatusFileSize
new 3185728-11.patch4.52 KB
new interdiff_4-11.txt4.07 KB

uploading the new patch with issue fixes. Kindly review.

dww’s picture

dww
we/he/they
commented
StatusFileSize
new 3185728-12.git-fork-screenshot.png42.73 KB

@vinaik thanks! Would you be willing to add your changes to the existing Git fork for this issue?

https://git.drupalcode.org/issue/drupal-3185728/-/tree/3185728-create-ti...

See https://www.drupal.org/docs/develop/git/using-git-to-contribute-to-drupa... for docs.

Scroll up to the top of the issue to see this info:

screenshot of git instructions for this issue

Thanks again,
-Derek

vinaik opened merge request !805

vinai_katiyar’s picture

vinai_katiyar commented

@ Derek Thanks for your suggestion. It's my first time to work with Git fork issue. I have pushed the changes to the existing Git fork for this issue but getting errors. Kindly help me .

Thanks,

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3185728-nr-bot.txt149 bytes

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. 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.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.