-
Notifications
You must be signed in to change notification settings - Fork 278
add unittest to test problems page before contset start #2756
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
Conversation
0e12edd to
c524217
Compare
| $timezone = substr($originalStartTime, $lastSpacePosition + 1); | ||
|
|
||
| $date = new \DateTime($datetime, new \DateTimeZone($timezone)); | ||
| $date->modify('+1 hour'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you do this with a Fixture like https://github.com/DOMjudge/domjudge/blob/main/webapp/src/DataFixtures/Test/DemoAboutToStartContestFixture.php? That makes it much easier to see what kind of state this test runs from.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see the Fixture yet. Did you push the change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've taken care of it. I just misunderstood you.
| static::assertSelectorTextContains('.nav-item .nav-link.disabled', 'Problems'); | ||
| static::assertSelectorTextContains('.alert.alert-secondary', 'No problem texts available at this point.'); | ||
|
|
||
| $this->client->request('GET', '/team/problems/2/statement'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you do a loop here? And I wonder if /team/problems/2 should also throw a 404 as it would disclose the number of problems already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I directly wrote a loop from one to three to test all three problems. Was this necessary, or is there somewhere that reveals the number of problems that I missed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could ask the ContestProblems from the Contest and loop over that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it okay for me to do this now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think its really better to get the problems from the contest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dealt with it
f02f2db to
c9fc4a4
Compare
c8bfada to
ae65428
Compare
| use App\Entity\Problem; | ||
| use App\Entity\ContestProblem; | ||
| use App\Entity\Testcase; | ||
| use App\Entity\Contest; | ||
| use App\Tests\Unit\BaseTestCase; | ||
| use Doctrine\ORM\EntityManagerInterface; | ||
| use App\DataFixtures\Test\DemoAboutToStartContestFixture; | ||
| use Generator; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We sort here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
| $em = self::getContainer()->get(EntityManagerInterface::class); | ||
| $problemCount = $em->createQueryBuilder() | ||
| ->from(ContestProblem::class, 'cp') | ||
| ->select('COUNT(cp.contest)') | ||
| ->where('cp.contest = :contest') | ||
| ->setParameter('contest', 1) | ||
| ->getQuery() | ||
| ->getSingleScalarResult(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you get the contest by its name, not by the internal number which can change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
ae65428 to
38b4ab7
Compare
| static::assertSelectorTextContains('.nav-item .nav-link.disabled', 'Problems'); | ||
| static::assertSelectorTextContains('.alert.alert-secondary', 'No problem texts available at this point.'); | ||
|
|
||
| for ($i = 1; $i <= $problemCount; $i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't hardcode the $i here as I think we use the global problemId and not the index in the contest, so when there are multiple problems in the database we might pick the wrong one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to use contest_id to fetch pid from the problem.
38b4ab7 to
2599762
Compare
vmcj
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's see if the rest has opinions and otherwise I'll merge next week.
…ment, sample data before contest start.
2599762 to
857338e
Compare
issue
#2393