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.

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

Spot TDbCronModuleTest test failure #918

Closed
ctrlaltca opened this issue Apr 27, 2023 · 7 comments
Closed

Spot TDbCronModuleTest test failure #918

ctrlaltca opened this issue Apr 27, 2023 · 7 comments

Comments

@ctrlaltca
Copy link
Member

Sometimes a TDbCronModuleTest unit test fails.

1) TDbCronModuleTest::testLogCronTask
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'testTaskAA'
+'testTaskDD'

/home/runner/work/prado/prado/tests/unit/Util/Cron/TDbCronModuleTest.php:246
/home/runner/work/prado/prado/vendor/bin/phpunit:123

You can see an example here: https://github.com/pradosoft/prado/actions/runs/4822368307/jobs/8589506193

@ctrlaltca ctrlaltca changed the title Spot test failure Spot TDbCronModuleTest test failure Apr 27, 2023
@belisoful
Copy link
Member

I've noticed this. I have a fix for this in the up coming Cron v2 update... I've been wildly distracted from that portion... but it's still waiting in the wings.

I think this issue come from an SQL order that isn't happening and two elements are rarely "randomly" gets switched. The unit test could be "corrected" to look for elements in not just static places... but the new Cron code should fix this.

@belisoful
Copy link
Member

belisoful commented Apr 27, 2023

#833 I do believe this fixes the issue... But it isn't complete yet. Getting the behaviors updated was very high priority, while cron was much lower. I was hoping to have this updated quickly after the last cron time fix but went down a huge rabbit hole.

@belisoful
Copy link
Member

Let me clarify. The presumption in DB Cron is that when a DB record is inserted, it will be returned in order of insertion and so the SQL is not ordered by Record UID in getting the records because of this presumption.

As we are seeing, the DB is not always returning the same order as insertion. I don't know why. However, the new Cron V2 does order by insertion id in this case.

@belisoful
Copy link
Member

If you are looking for a quick fix, one of the SQL SELECT statements "just" needs to be ordered by the UID. I'm sure it would be easy to nail down given the unit test that is failing and seeing the cron method producing the wrong results.

Besides properly ordering by UID, Cron v2 lazy loads the jobs from the DB rather than loads them all. Only those needed are loaded. It can handle a much larger load of cron jobs. That is to say, it makes the cron system ready for a user base rather than "just for system use" or low usage.

belisoful added a commit to belisoful/prado that referenced this issue Apr 27, 2023
…ailure

I didn't think this was necessary because.... wouldn't the DB return the table in the order of insertion?

apparently, randomly, it switches rows unless explicitly ordered
@belisoful
Copy link
Member

I went ahead and zinged this spot error. I do believe this fixed the issue.

ctrlaltca pushed a commit that referenced this issue Apr 28, 2023
)

I didn't think this was necessary because.... wouldn't the DB return the table in the order of insertion?

apparently, randomly, it switches rows unless explicitly ordered
@belisoful
Copy link
Member

belisoful commented May 3, 2023

ok. didn't fix the issue. This time:

1) TDbCronModuleTest::testLogCronTask
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'testTaskAA'
+'testTaskBB'

/2022prado/protected/vendor/pradosoft/prado/tests/unit/Util/Cron/TDbCronModuleTest.php:246
/2022prado/protected/vendor/pradosoft/prado/vendor/bin/phpunit:123

@belisoful belisoful reopened this May 3, 2023
belisoful added a commit to belisoful/prado that referenced this issue May 3, 2023
The test itself wasn't quite right.  it should sort by asc (false), not descending (true). and ordering by uid (aka entry order) as well.
ctrlaltca pushed a commit that referenced this issue May 4, 2023
The test itself wasn't quite right.  it should sort by asc (false), not descending (true). and ordering by uid (aka entry order) as well.
@belisoful
Copy link
Member

ok. NOW I'm sure it's fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants