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

Run integration tests on current master #456

Closed
carlosms opened this issue Apr 24, 2019 · 14 comments
Closed

Run integration tests on current master #456

carlosms opened this issue Apr 24, 2019 · 14 comments
Assignees

Comments

@carlosms
Copy link
Contributor

Code is ready for 0.13 rc1, we need to run the tests on the windows dev machine.
And also manually test:

  • If new log output from go-log looks ok
  • If srcd config works as expected. It should be able to open the default editor, notepad.
@smacker smacker self-assigned this Apr 24, 2019
@smacker
Copy link
Contributor

smacker commented Apr 24, 2019

I have fixed some bugs & merged in master as you could see.

But we have more problems:

  • go-cli uses /flag format for options instead of --flag as it was before, are we ok with it?
  • we have changed how errors are displayed. Now they escape \ and tests are failing
  • some sql tests are failing now. Because queries like /*_comment_*/_show_tables; are parsed like unknown flags now. What do we want to do about it?
  • TestWebTestSuite/TestSQL fails with could not prune components: unable to remove all containers: container not found now sure why
  • logs are ugly now:

Screenshot 2019-04-24 at 18 07 26

- looks like `StreamLinifier` doesn't support windows correctly (most probably due to `\r\n` instead of '\n'). Tests are failing.

@carlosms
Copy link
Contributor Author

  • go-cli uses /flag format for options instead of --flag as it was before, are we ok with it?
  • some sql tests are failing now. Because queries like /*_comment_*/_show_tables; are parsed like unknown flags now. What do we want to do about it?

From jessevdk/go-flags docs, we can change it to be --opt with the build tag forceposix.
We can change it in run-integration-tests.bat (go build -tags forceposix) and Makefile (GO_TAGS = forceposix)

  • TestWebTestSuite/TestSQL fails with could not prune components: unable to remove all containers: container not found now sure why

This one looks like the most important problem to me.

@se7entyse7en
Copy link
Contributor

  • go-cli uses /flag format for options instead of --flag as it was before, are we ok with it?
  • some sql tests are failing now. Because queries like /*_comment_*/_show_tables; are parsed like unknown flags now. What do we want to do about it?

If the windows user is more familiar with / instead of - I'd keep this behavior. WRT the tests I think that it makes sense to have different test builds for Windows. In this /*_comment_*/_show_tables; case on windows is actually expected that it raises an unknown flags error.

  • looks like StreamLinifier doesn't support windows correctly (most probably due to \r\n instead of '\n'). Tests are failing.

This seems to be an easy fix.

  • TestWebTestSuite/TestSQL fails with could not prune components: unable to remove all containers: container not found now sure why

This one looks like the most important problem to me.

Yup, I agree 😕.

@carlosms
Copy link
Contributor Author

  • go-cli uses /flag format for options instead of --flag as it was before, are we ok with it?
  • some sql tests are failing now. Because queries like /*_comment_*/_show_tables; are parsed like unknown flags now. What do we want to do about it?

If the windows user is more familiar with / instead of - I'd keep this behavior.

If this was the first windows release, it would make sense. But in this case it is a breaking change that we can avoid easily, and as an extra it probably takes less effort to add this build tag than adapt the tests to work differently on windows.

@se7entyse7en se7entyse7en assigned se7entyse7en and unassigned smacker Apr 25, 2019
@se7entyse7en
Copy link
Contributor

If this was the first windows release, it would make sense. But in this case it is a breaking change that we can avoid easily.

Isn't it the opposite? I mean we already have a released version (though experimental), so if we use the forceposix tag we will break the usage of / that is how it works now.

@smacker
Copy link
Contributor

smacker commented Apr 25, 2019

Nope. In the previous release, we used cobra instead of go-cli. It uses posix style by default and doesn't support /. I agree with Carlos on forcing posix in go-cli to keep behavior the same as before.

@carlosms
Copy link
Contributor Author

If this was the first windows release, it would make sense. But in this case it is a breaking change that we can avoid easily.

Isn't it the opposite? I mean we already have a released version (though experimental), so if we use the forceposix tag we will break the usage of / that is how it works now.

But 0.11 and 0.12 already use the --opt style. If we change it to /opt in 0.13 it's a breaking change.

@se7entyse7en
Copy link
Contributor

But 0.11 and 0.12 already use the --opt style. If we change it to /opt in 0.13 it's a breaking change.

Yup, sorry I tried on master 😅.

@se7entyse7en
Copy link
Contributor

BTW it seems that now both options are working, so it wouldn't be a real breaking change except for the fact that it won't be explained in the usage.

@se7entyse7en
Copy link
Contributor

#459 addresses all issues except for the ugly log.

  • looks like StreamLinifier doesn't support windows correctly (most probably due to \r\n instead of '\n').

AFAIR not it is used only for interactive repl testing, but they're skipped for Windows.

@smacker
Copy link
Contributor

smacker commented Apr 25, 2019

StreamLinifier

but when I run tests they failed on windows. Not tests that use this struct but the test for the struct itself.

@se7entyse7en
Copy link
Contributor

se7entyse7en commented Apr 25, 2019

Not tests that use this struct but the test for the struct itself.

Ah ok, right! 😅 For some reason they're passing now 👍, I don't remember if they were failing when I started working on this.

@smacker
Copy link
Contributor

smacker commented Apr 26, 2019

Fix for logs was merged in go-log.

@carlosms
Copy link
Contributor Author

Everything seems to work now except the log colors. The code in src-d/go-log#16 did not fix the output in Engine.
I created #462 to keep track, we can close this issue.

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