cognoma / frontend

Frontend for Project Cognoma
http://cognoma.org/
Other
4 stars 22 forks source link

Better karma config and unit testing process #87

Closed bdolly closed 7 years ago

bdolly commented 7 years ago
dhimmel commented 7 years ago

@arielsvn feel free to review this.

@bdolly, @arielsvn is a web developer who is helping with some Greene Lab projects and may be interested in doing some Cognoma frontend contributing.

bdolly commented 7 years ago

@dhimmel can't figure out how to merge this. I've pulled the PR down and tried to merge via command line but no luck.

This PR should resolve the timeouts in the CircleCI builds

arielsvn commented 7 years ago

Looks good to me, although I don't now much about Angular and I have to become more familiar with the project.

After a quick check only one thing caught my attention... why is the ngstorage package part of the source code?? under the directory /app/js/utils. I saw a commit that moved it from from being an npm package to the source code here https://github.com/cognoma/frontend/commit/aefedd98642afd63b97b377f8df9a1893c994a58

dhimmel commented 7 years ago

This PR should resolve the timeouts in the CircleCI builds

Cool, I added Closes #86 to your original comment, so GitHub automatically closes that issue when this is merged.

can't figure out how to merge this

You'll need to fix the conflict first in app/js/components/queryBuilder/queryParamSelector/mutations_instructions.tpl.html. You will have to retrieve the latest version of cognoma:develop locally and then either:

  1. rebase bdolly:feature/better-karma-config on the current cognoma:develop
  2. merge the current cognoma:develop into bdolly:feature/better-karma-config.

Solution 1 is preferred but a but requires a force push to bdolly:feature/better-karma-config. Both solutions will reach a point where you need to manually fix the conflict in mutations_instructions.tpl.html. You can do this in a text editor and then continue.

Good luck!

bdolly commented 7 years ago

@dhimmel I followed the rebase method and I think commit 54204e1 resolved it. Can you take a look just to double check?

dhimmel commented 7 years ago

https://github.com/cognoma/frontend/commit/96dca3d8eddf77a441587b2b5493c257c582e2be looks like it committed the unresolved conflict and https://github.com/cognoma/frontend/commit/a30f0cc1004656314881788163b7ace117943c7f chose which version to use. The visualization here can also help decipher the history.

@bdolly since you were able to merge it, I think we're good regarding the conflict. The last build (for https://github.com/cognoma/frontend/commit/a30f0cc1004656314881788163b7ace117943c7f) timed out, so I'm not sure #86 is actually solved yet.