Skip to content

Conversation

@bgee
Copy link

@bgee bgee commented Apr 25, 2013

Add import speed test using bin/test_import.py.

sympy-bot Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a space after the 🕙 to make the bold work.

sympy-bot Outdated
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't even right. It should be bin/test_import

@asmeurer
Copy link
Member

You should parse the output of test_import and use the result from there (with the +-).

@bgee
Copy link
Author

bgee commented Apr 25, 2013

@asmeurer Just did. sympy/sympy#1924
Also, test_import just assumes it will be called inside sympy/. Should I fix this in sympy? No big deal though.
(Sorry I accidentally closed this pull request. )

@bgee bgee closed this Apr 25, 2013
@bgee bgee reopened this Apr 25, 2013
@asmeurer
Copy link
Member

Yes, fix it. Look at all the other various functions to see how (e.g., bin/test).

@asmeurer
Copy link
Member

Could you add a flag to make this configurable (both command line and config file)?

@bgee
Copy link
Author

bgee commented Apr 25, 2013

@asmeurer Yes. I will try to finish this before tomorrow afternoon. What will be a good flag? We already have -t. Should we make it default to run?
Also, I saw the test you put has a line Test command: ** -V**. It's not showing up on my machine. Did I break anything?

@asmeurer
Copy link
Member

No, I tested with -t " -V" so that it wouldn't actually run the tests, so that it would come up quickly.

@asmeurer
Copy link
Member

I guess call it --import-time or --no-import-time (depending on what you make the default).

@bgee
Copy link
Author

bgee commented Apr 26, 2013

@asmeurer I use --no-import-time for command line option and no_import_time = false/true in config file.

@asmeurer
Copy link
Member

This looks fine. I want to just wait a little bit (maybe a few days) to see if anyone else has any comments.

@asmeurer
Copy link
Member

By the way, you can test comments without spamming a pull request by using --no-comment (-n is also a good idea, since there's no point in uploading the reviews either), then pasting the comment output that is printed to the terminal in an issue in some test GitHub repository (I use https://github.com/asmeurer/GitHub-Issues-Test/issues).

@bgee
Copy link
Author

bgee commented Apr 26, 2013

@asmeurer Sounds good. I will also start working on that bin/test_import directory fix.

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.

2 participants