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

Webapp fix #397

Merged
merged 5 commits into from
Sep 21, 2016
Merged

Webapp fix #397

merged 5 commits into from
Sep 21, 2016

Conversation

bhargav
Copy link
Contributor

@bhargav bhargav commented Sep 21, 2016

  • Updated content for Email Spam, Drug Response and Edison Features app.
  • @danyaljj fixed the issue with Logger error during compilation.

Pending:

  • Webapp unit tests are failing. Will investigate today.

Towards #381

@danyaljj
Copy link
Member

Looks good to me.

@kordjamshidi
Copy link
Member

I think: please make the main fix which is about failing tests before merge.

@danyaljj
Copy link
Member

danyaljj commented Sep 21, 2016

This PR will make the webapp temporarily functional.
Current webapp has significant issues that need to be addressed (e.g. #398) as well as the better tests to ensure the correct functionality.
I would suggest moving on with this PR, and addressing other issues in future PRs, step by step (as long term goals).

@kordjamshidi
Copy link
Member

kordjamshidi commented Sep 21, 2016

are the tests failing due to #398 ?

@danyaljj
Copy link
Member

are the test failing due to #398 ?

The tests are not failing because of that issue. They are failing due to some injection issue (which I am not sure how to fix). But that is not the point; Look at the current tests: https://github.com/IllinoisCogComp/saul/tree/master/saul-webapp/test
Currently our webapp tests, have literally NOTHING. Even if these tests work, they ensure NOTHING.
We first have to add good/high-coverage test. And if we don't fix #398, we won't be able to have any meaningful tests.

@kordjamshidi
Copy link
Member

Okaayy.

@kordjamshidi kordjamshidi merged commit 310ca43 into CogComp:master Sep 21, 2016
@bhargav bhargav deleted the webapp-content-fix branch October 6, 2016 21:55
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

Successfully merging this pull request may close these issues.

3 participants