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

Fixing type inference tests #48

Open
madmann91 opened this issue Feb 6, 2017 · 5 comments
Open

Fixing type inference tests #48

madmann91 opened this issue Feb 6, 2017 · 5 comments
Milestone

Comments

@madmann91
Copy link
Contributor

Many tests for impala are broken because we currently print types such as fn(i32) -> i32 as fn(i32, fn(i32)). The code that did the pretty printing previously is now commented out in src/impala/sema/type.cpp. Should we change the tests and get a rid of the commented code, or should we re-enable the pretty printing ?

@leissa
Copy link
Member

leissa commented Feb 6, 2017

We should fix pretty printing.

That being said, most sema tests are broken anyway since they have not been ported after merging the infer branch. Too many things changed...

If you want to update the sema tests, the best way would be:

  • discard all test cases which we currently not support (in particular polymorphism, traits, etc)
  • write a script which updates the expected output

@stlemme
Copy link
Member

stlemme commented Feb 6, 2017

Beside fixing tests, I suggest to move all tests into an AnyDSL/testsuite.git repo and ensure, that all tests ported there are known to pass. This would ease to automate testing.

@leissa
Copy link
Member

leissa commented Feb 6, 2017

IMHO tests belong to the stuff they are testing. Most tests in impala/tests are actually tests for impala. However, there are a couple of tests which actually test things in thorin. Unfortunately, we haven't an infrastructure in place to test thorin without impala.

What advantages would you have from a separate testing repo? I only see disadvantages: For example, you have to pair the testing repo revision with the corrent impala repo revision.

@stlemme
Copy link
Member

stlemme commented Feb 6, 2017

Mainly, because it doesn't matter which IMPALA_BIN you use for testing. You could even use the docker image of anydsl/impala. Moreover, you get rid of the outdated stuff and ensure the testsuite always works against impala@master.

Since there are already tests for thorin, you would actually need to pair thorin and impala today. However, we do not enforce that.

I would even expect more tests (i.e. integration tests rather than unit tests) for instance to validate the runtime works correctly.

@leissa
Copy link
Member

leissa commented Feb 6, 2017

I think we should put this discussion offline, and discuss that on our next meeting - we're getting a bit off-topic :)

@leissa leissa added this to the 0.4 milestone Feb 13, 2017
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

No branches or pull requests

3 participants