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

Implement support for long intervals #53

Merged
merged 9 commits into from
Oct 31, 2021

Conversation

AndreWillomitzer
Copy link
Contributor

@AndreWillomitzer AndreWillomitzer commented Oct 29, 2021

fixes #24

@AndreWillomitzer
Copy link
Contributor Author

@kibertoad
Hi there boss, I made some changes to the PR. I ran npm test, and it passes 3, fails 3 but I don't know which ones. And it also says 0 of 5 suites? I tried to edit the jest config file and use "it.only()" but no luck. Do you know how I can run just the long interval test?
image

Also, in the initial commit I was having the issue "Kaboom" which prompted me to make the changes.
image

Any advice on what to change would be great, I would like to help finish this off!!! 👍

@AndreWillomitzer
Copy link
Contributor Author

@kibertoad Basically at this point I'm pretty stuck. I don't exactly know why it's hanging on the test and doesn't stop counting until I terminate the batch job. With the most recent commit I didn't see the "kaboom" error anymore but it passes 3 tests and fails 3 so something isn't right but the test doesn't ever complete to tell me what's wrong or what failed.

I pretty much have until Sunday midnight to try and fix this or at least merge my PR into a draft branch in your repo so that you could continue working on it.

@AndreWillomitzer AndreWillomitzer marked this pull request as ready for review October 29, 2021 17:33
@AndreWillomitzer
Copy link
Contributor Author

@kibertoad
Hope all is well I bet you're busy and I feel bad messaging ya but with my professors help we managed to modify the constructor code, and now when I run the test for long intervals it says 1 of 1 passed. If I run all the tests it says 3 of 6 passed 3 failed. Would you be able to help me inspect why it fails the other tests?
image

It passes the test and then kind of just hangs there in the terminal like it's waiting for the task, even though I used advanceTimersByTime().

All the best,

Andre

@kibertoad
Copy link
Owner

Hey, Andre!
Sorry for the delay in replying. Will take a look on it tonight!

@AndreWillomitzer
Copy link
Contributor Author

@kibertoad Hi there sir. Sadly it seems I have run out of time for Hacktoberfest to work on this issue. Would you be willing to make an "issue branch" for this PR and merge it into there?

That way the work we did will stick around and can be used later :)

Thanks,
Andre

@kibertoad
Copy link
Owner

@AndreWillomitzer You've actually implemented 90% of the task at hand, I'm doing some final fixes to it right now, I think we can still merge it before midnight :)

@AndreWillomitzer
Copy link
Contributor Author

@kibertoad Oh wow really?! I'm over the moon that what I got going was useful haha! That's really exciting.

Thanks for being cool about everything, our comment thread ended up 47 long lol. You and my prof were a great help.

Going to keep my peepers open to see what you implement for sure out of curiosity.

@kibertoad
Copy link
Owner

@AndreWillomitzer I think we are almost there, but final task is not firing for me for some reason :-/. I need to go now, but will be back in two hours or so. If you'll have time to take a look at what could still be wrong, that would be helpful, but if you are busy, no worries, I think I should be able to fix it today either way.

@kibertoad
Copy link
Owner

I think I managed to fix it in the last moment :D.
Can you please write documentation for new LongIntervalJob in readme.md?

@AndreWillomitzer
Copy link
Contributor Author

@kibertoad Ahhhh I see what you did with the start/stop inside the constructor. I guess I forgot those key things about the jobs. As for documentation what did you have in mind? Most of what gets handled probably has nothing to do with the user doing anything it's all behind the scenes isn't it?

I too have to disappear for a bit but will be back tonight if you still need the README done :D But if you want to merge the PR and do it too that's fine with me!

Thanks!

@kibertoad kibertoad changed the title added initial longInterval constructor code/tests Implement support for long intervals Oct 31, 2021
@kibertoad kibertoad merged commit 9e058f4 into kibertoad:main Oct 31, 2021
@AndreWillomitzer
Copy link
Contributor Author

@kibertoad Thanks again for letting me work on this it was a super super cool experience and a unique issue to solve haha, probably something I'll remember forever honestly. It was a lot of brain-cramping to get to the solution but so worth it.

If you need any help in the future I am sure we will have to commit something to a repo in the future so I'll keep my eyes open :D

All the best,

Andre!

@kibertoad
Copy link
Owner

@AndreWillomitzer It was a pleasure! You definitely are going to have bright future in software engineering. And I hope that we get to collaborate again, on this project or some other!

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

Successfully merging this pull request may close these issues.

Scheduling breaks completely on long intervals
2 participants