search

Plant-Tracer / webapp

Client and Server for web-based JavaScript app
GNU Affero General Public License v3.0
0 stars 2 forks source link

[DRAFT] Adding Jest-based testing for PlantTracer.js and its associated functions PR #457

Closed joedinsmoor closed 1 month ago

joedinsmoor commented 4 months ago

As of now, a working framework is set up to test the list_movies() function within PlantTracer. If everybody is satisfied with my approach, I'll continue adding tests for the remaining functions within PlantTracer. I can either segment to individual files for each function, or create a massive test file, whichever is preferred.

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 84.40%. Comparing base (8457310) to head (c10c4b6). Report is 5 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #457 +/- ## ======================================= Coverage 84.40% 84.40% ======================================= Files 23 23 Lines 2731 2731 ======================================= Hits 2305 2305 Misses 426 426 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

simsong commented 4 months ago

Neat. I will review. Also interested in chatting with you about what I’ve learned about Apache SAM.

On Fri, Jul 12, 2024 at 11:05 AM Joe Dinsmoor @.***> wrote:

As of now, a working framework is set up to test the list_movies() function within PlantTracer. If everybody is satisfied with my approach, I'll continue adding tests for the remaining functions within PlantTracer. I can either segment to individual files for each function, or create a massive test file, whichever is preferred.

You can view, comment on, or merge this pull request online at:

https://github.com/Plant-Tracer/webapp/pull/457 Commit Summary

File Changes

(6 files https://github.com/Plant-Tracer/webapp/pull/457/files)

Patch Links:

— Reply to this email directly, view it on GitHub https://github.com/Plant-Tracer/webapp/pull/457, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMFHLGNDWZOD7R7NDBABNDZL7WB7AVCNFSM6AAAAABKZDB3YSVHI2DSMVQWIX3LMV43ASLTON2WKOZSGQYDKOBRHE3TONY . You are receiving this because you are subscribed to this thread.Message ID: @.***>

sbarber2 commented 2 months ago

Hmm. I had no idea @joedinsmoor was also working on cleaning up docs and I already made and merged very similar changes via PR #470 a couple weeks ago

Just a note that attempting to merge the files in this PR is going to create many merge conflicts in docs.

Also be aware that we're in the process of making changes to the List Movies page in demo mode PR # 522 / Issue #521, so that may affect the current content of this PR, not sure.

(Though I'm quite excited about the JS testing. The reason I was even looking at this PR was that I was thinking about writing some tests maybe for #521 and it occurred to me that maybe this pending Jest-based testing might be close to ready.)

So, what's next for this PR? Also, this really wasn't on my radar as WIP for our weekly dev meetings, so that's something to fix. I'll add this, as it's important.

joedinsmoor commented 2 months ago

I'll leave out the documentation changes I made earlier in this PR and defer to you. As far as the rest of it, I'm slowly wrapping up test writing for the remainder of the PlantTracer.js module. It's been slow due to other things on my plate. I'll make sure to keep you and @simsong in the loop.

sbarber2 commented 2 months ago

Thanks.

I'll leave out the documentation changes I made earlier in this PR and defer to you.

No deference necessary. Coordination, though, that we could use. Probably my bad for not paying attention to this since June.

The doc changes we made to make things more RST-ish and the install/setup docs probably are probably largely duplicative (I haven't scrutinized them, but just my impression from a quick once over), but the substance changes to the movie player docs here, for example, are additive and useful so why throw them away?

I was mostly just giving you a heads-up that there would be a bunch of merge conflicts.

joedinsmoor commented 1 month ago

Hi, so three tests are good and ready to go into production. I still need clarity on usage limits for the upload movie function before that can be completed. If it is wanted, I can open a separate issue and PR for the remaining test so that the completed ones can be used.

sbarber2 commented 1 month ago

I still need clarity on usage limits for the upload movie function before that can be completed.

Hmm, yeah, you mentioned that somewhere before. Sorry, I lost track of that question.

Not to be didactic, but procedurally it would have been nice to have this question in an issue assigned to the person you think is best positioned to resolve it.

If it is wanted, I can open a separate issue and PR for the remaining test so that the completed ones can be used.

Yes, please separate it. You can include your upload usage limit query in the Issue and then assign it to someone, using the question or needs data label.

In the meantime, I'm dodging the substantive question and deflecting it with procedure mostly because I don't know the answer. Simson can weigh in, and if he doesn't know, I will do my best to make something up. Simson I think has a very direct interest in the question since it's his AWS account we're using!

-Steve

On Thu, Oct 10, 2024 at 1:11 PM Joe Dinsmoor @.***> wrote:

Hi, so three tests are good and ready to go into production. I still need clarity on usage limits for the upload movie function before that can be completed. If it is wanted, I can open a separate issue and PR for the remaining test so that the completed ones can be used.

— Reply to this email directly, view it on GitHub https://github.com/Plant-Tracer/webapp/pull/457#issuecomment-2405643639, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFQOBZKEXXEXWDG7HMSU33Z22YLZAVCNFSM6AAAAABKZDB3YSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMBVGY2DGNRTHE . You are receiving this because you commented.Message ID: @.***>

joedinsmoor commented 1 month ago

I still need clarity on usage limits for the upload movie function before that can be completed. Hmm, yeah, you mentioned that somewhere before. Sorry, I lost track of that question. Not to be didactic, but procedurally it would have been nice to have this question in an issue assigned to the person you think is best positioned to resolve it. If it is wanted, I can open a separate issue and PR for the remaining test so that the completed ones can be used. Yes, please separate it. You can include your upload usage limit query in the Issue and then assign it to someone, using the question or needs data label. In the meantime, I'm dodging the substantive question and deflecting it with procedure mostly because I don't know the answer. Simson can weigh in, and if he doesn't know, I will do my best to make something up. Simson I think has a very direct interest in the question since it's his AWS account we're using! … -Steve On Thu, Oct 10, 2024 at 1:11 PM Joe Dinsmoor @.> wrote: Hi, so three tests are good and ready to go into production. I still need clarity on usage limits for the upload movie function before that can be completed. If it is wanted, I can open a separate issue and PR for the remaining test so that the completed ones can be used. — Reply to this email directly, view it on GitHub <#457 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAFQOBZKEXXEXWDG7HMSU33Z22YLZAVCNFSM6AAAAABKZDB3YSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMBVGY2DGNRTHE . You are receiving this because you commented.Message ID: @.>

Understood, I'll open the new issue is open #554

sbarber2 commented 1 month ago

Also, happy to help if the path forward is not clear.

joedinsmoor commented 1 month ago

Closing this PR and moving to #555 to avoid any merge errors I may have incidentally created

simsong commented 1 month ago

Another simson typo.

On Oct 10, 2024, at 1:34 PM, Steve Barber @.***> wrote:

@sbarber2 requested changes on this pull request.

Looks like this PR wasn't correctly merged back from main. Please review and fix.

In Makefile https://github.com/Plant-Tracer/webapp/pull/457#discussion_r1795847901:

@@ -71,12 +73,12 @@ flake:

Dynamic Analysis

pytest-db: $(REQ)

  • if [ -z "$${PLANTTRACER_CREDENTIALS}" ]; then echo PLANTTRACER_CREDENTIALS is not set; exit 1; fi
  • if [ -z "$${PLANTTRACER_CREDENTIALS}" ]; then echo PLANTTRACER_CREDETIALS is not set; exit 1; fi Again, you are backing out changes made in main with this PR. OK, I'm going to stop reviewing now and send this back to you.

— Reply to this email directly, view it on GitHub https://github.com/Plant-Tracer/webapp/pull/457#pullrequestreview-2361012195, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAMFHLCQU5BDI6OFUCMELSLZ223CTAVCNFSM6AAAAABKZDB3YSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGNRRGAYTEMJZGU. You are receiving this because you were mentioned.

sbarber2 commented 1 month ago

😀