Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Tests that rely on Order.EMPTY are time-sensitive #6636

Open
wzieba opened this issue May 27, 2022 · 4 comments · May be fixed by #13478
Open

Tests that rely on Order.EMPTY are time-sensitive #6636

wzieba opened this issue May 27, 2022 · 4 comments · May be fixed by #13478
Labels
category: unit tests Related to unit testing. feature: order creation Related to the Order Creation feature feature: order details Related to order details. type: bug A confirmed bug.

Comments

@wzieba
Copy link
Contributor

wzieba commented May 27, 2022 •
edited
Loading

All unit tests that use Order.EMPTY are now based on time, as since #6534 the time of creation is generated on each get.

This means that when executing tests, we can't compare Order.EMPTY as it'll be different each time it's requested.

IMO we should make Order.EMPTY time agnostic (assign a default value for date, e.g. Instant.EPOCH) and introduce injectable java.time.Clock which will be a fixed one in tests.

Also maybe it's a naming issue. Because Order.EMPTY.isEmpty() now returns false.
@wzieba wzieba added type: bug A confirmed bug. category: unit tests Related to unit testing. feature: order details Related to order details. feature: order creation Related to the Order Creation feature labels May 27, 2022
@Akshaykomar890
Copy link
Contributor

Hi @wzieba,

I noticed this issue and find the idea of making Order.EMPTY time agnostic very interesting. Introducing a default date value like Instant.EPOCH and an injectable java.time.Clock for fixed values during tests sounds like a great approach to improve consistency and test reliability.

Would it be alright if I take this up? I'd be happy to work on implementing this and addressing the naming concern regarding Order.EMPTY.isEmpty().

@wzieba
Copy link
Contributor Author

wzieba commented Jan 13, 2025

hi @Akshaykomar890 👋 thanks for being interested in improving this. As I'm not working on this project actively anymore, let me cc @malinajirka to provide their insight here.

@malinajirka
Copy link
Contributor

Hello again @Akshaykomar890 👋,

thanks again for your interest in contributing! Feel free to work on this issue.

I don't have much insights into the complexity of this issue, but I like the idea and the solution proposed by Wojtek sounds like a good one. Let me know in case you encounter any issues and I'll be happy to chat about them with you (and hopefully help you resolve them :P ).

Akshaykomar890 added a commit to Akshaykomar890/woocommerce-android that referenced this issue Feb 5, 2025
@Akshaykomar890
Implementation of a Time-Agnostic Order.EMPTY
224d8cb 
fix : woocommerce#6636

I implemented the Order class is refactored to remove time sensitivity in tests by using a fixed timestamp for the Order.EMPTY instance. Instead of generating dynamic timestamps, dateCreated and dateModified are assigned constant values using Date.from(Instant.EPOCH), which corresponds to the Unix epoch (January 1, 1970). This ensures that comparisons in tests remain consistent and predictable.
@Akshaykomar890
Copy link
Contributor

Akshaykomar890 commented Feb 5, 2025 •
edited
Loading

Hello @malinajirka,

Thanks for your previous feedback and support.
#13478 I've implemented the proposed changes and submitted my PR. Could you please take a look and review it when you have a moment? Let me know if there are any issues or further adjustments needed.

Thanks again

@Akshaykomar890 Akshaykomar890 linked a pull request Feb 5, 2025 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
category: unit tests Related to unit testing. feature: order creation Related to the Order Creation feature feature: order details Related to order details. type: bug A confirmed bug.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Implementation of a Time-Agnostic Order.EMPTY Akshaykomar890/woocommerce-android
3 participants
@malinajirka @wzieba @Akshaykomar890