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

Using empty() to check for a set db #13699

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@ryanml
Contributor

ryanml commented Sep 21, 2017

Signed-off-by: Ryan Lanese rlanese@asu.edu

Before submitting pull request, please check that every commit:

  • Has proper Signed-Off-By
  • Has commit message which describes it
  • Is needed on it's own, if you have just minor fixes to previous commits, you can squash them
  • Any new functionality is covered by tests
Using empty() to check for a set db
Signed-off-by: Ryan Lanese <rlanese@asu.edu>
@ryanml

This comment has been minimized.

Show comment
Hide comment
@ryanml

ryanml Sep 21, 2017

Contributor

@mauriciofauth I opened a new PR using empty, as I made I pushed a few incorrect commits to the old one by mistake (I figured this was easier) [ref PR #13698]

I typically haven't used empty due to differences in behavior across PHP versions, but you're right in that it's better for readability, and will likely be fine for the purposes of this fix.

Contributor

ryanml commented Sep 21, 2017

@mauriciofauth I opened a new PR using empty, as I made I pushed a few incorrect commits to the old one by mistake (I figured this was easier) [ref PR #13698]

I typically haven't used empty due to differences in behavior across PHP versions, but you're right in that it's better for readability, and will likely be fine for the purposes of this fix.

@ryanml

This comment has been minimized.

Show comment
Hide comment
@ryanml

ryanml Sep 21, 2017

Contributor

@mauriciofauth Also to note, this will fix issue #12856 as well as #13490

Contributor

ryanml commented Sep 21, 2017

@mauriciofauth Also to note, this will fix issue #12856 as well as #13490

@mauriciofauth

This comment has been minimized.

Show comment
Hide comment
@mauriciofauth

mauriciofauth Sep 21, 2017

Member

This change broke one of the Selenium tests. See:

1) PMA_SeleniumImportTest::testServerImport
PHPUnit_Extensions_Selenium2TestCase_WebDriverException: no such element: Unable to locate element: {"method":"xpath","selector":"//div[@class='success' and contains(., 'Import has been successfully')]"}
Member

mauriciofauth commented Sep 21, 2017

This change broke one of the Selenium tests. See:

1) PMA_SeleniumImportTest::testServerImport
PHPUnit_Extensions_Selenium2TestCase_WebDriverException: no such element: Unable to locate element: {"method":"xpath","selector":"//div[@class='success' and contains(., 'Import has been successfully')]"}
@ryanml

This comment has been minimized.

Show comment
Hide comment
@ryanml

ryanml Sep 21, 2017

Contributor

@mauriciofauth I imagine the test case would have to change a bit to accommodate this behavior

I think a check like this would have to happen in PmaSeleniumImportTest.php on line 112:

if ($this->isElementPresent("byXPath", "//a[@class='item' and contains(., 'Database:')]"))

From the dom perspective, this looks at the top nav for the presence of a selected database, and ideally the test would only expect the success message if that exists

Does this seem like a reasonable adjustment to you? I appreciate your patience :)

Contributor

ryanml commented Sep 21, 2017

@mauriciofauth I imagine the test case would have to change a bit to accommodate this behavior

I think a check like this would have to happen in PmaSeleniumImportTest.php on line 112:

if ($this->isElementPresent("byXPath", "//a[@class='item' and contains(., 'Database:')]"))

From the dom perspective, this looks at the top nav for the presence of a selected database, and ideally the test would only expect the success message if that exists

Does this seem like a reasonable adjustment to you? I appreciate your patience :)

@mauriciofauth

This comment has been minimized.

Show comment
Hide comment
@mauriciofauth

mauriciofauth Sep 21, 2017

Member

What do you think about this, @devenbansod?

Member

mauriciofauth commented Sep 21, 2017

What do you think about this, @devenbansod?

@mauriciofauth

This comment has been minimized.

Show comment
Hide comment
@mauriciofauth

mauriciofauth Sep 21, 2017

Member

@ryanml Feel free to make changes to the tests. :)

Member

mauriciofauth commented Sep 21, 2017

@ryanml Feel free to make changes to the tests. :)

@ryanml

This comment has been minimized.

Show comment
Hide comment
@ryanml

ryanml Sep 21, 2017

Contributor

@mauriciofauth will do :) I'll be able to get to it this evening, thanks!

Contributor

ryanml commented Sep 21, 2017

@mauriciofauth will do :) I'll be able to get to it this evening, thanks!

@mauriciofauth

This comment has been minimized.

Show comment
Hide comment
@mauriciofauth

mauriciofauth Sep 22, 2017

Member

@ryanml about the new PR. If you push some incorrect commits or just minor fixes to previous commits, you can squash them with git rebase.

Member

mauriciofauth commented Sep 22, 2017

@ryanml about the new PR. If you push some incorrect commits or just minor fixes to previous commits, you can squash them with git rebase.

Changing import test to require a selected db before waiting for a su…
…ccessful import message

Signed-off-by: Ryan Lanese <rlanese@asu.edu>
@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Sep 22, 2017

Codecov Report

Merging #13699 into master will decrease coverage by 0.02%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master   #13699      +/-   ##
==========================================
- Coverage   53.84%   53.81%   -0.03%     
==========================================
  Files         480      481       +1     
  Lines       82135    82181      +46     
==========================================
+ Hits        44224    44228       +4     
- Misses      37911    37953      +42

codecov bot commented Sep 22, 2017

Codecov Report

Merging #13699 into master will decrease coverage by 0.02%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master   #13699      +/-   ##
==========================================
- Coverage   53.84%   53.81%   -0.03%     
==========================================
  Files         480      481       +1     
  Lines       82135    82181      +46     
==========================================
+ Hits        44224    44228       +4     
- Misses      37911    37953      +42
@ryanml

This comment has been minimized.

Show comment
Hide comment
@ryanml

ryanml Sep 22, 2017

Contributor

@mauriciofauth thanks for the tips :)

I updated the import test

It looks like everything is checking out with the selenium tests through Travis, are those code coverage checks anything I need to be concerned about?

Contributor

ryanml commented Sep 22, 2017

@mauriciofauth thanks for the tips :)

I updated the import test

It looks like everything is checking out with the selenium tests through Travis, are those code coverage checks anything I need to be concerned about?

@devenbansod

This comment has been minimized.

Show comment
Hide comment
@devenbansod

devenbansod Sep 22, 2017

Member

bs-video-logs-use.mp4.zip
I could not find time to go through the code completely, but this condition of database being empty might actually be true for a server import.

Also, as you can see the attached video, it actually gives an error in the test. So adding the if condition in the test does not seem like a good idea.

Member

devenbansod commented Sep 22, 2017

bs-video-logs-use.mp4.zip
I could not find time to go through the code completely, but this condition of database being empty might actually be true for a server import.

Also, as you can see the attached video, it actually gives an error in the test. So adding the if condition in the test does not seem like a good idea.

@ryanml

This comment has been minimized.

Show comment
Hide comment
@ryanml

ryanml Sep 23, 2017

Contributor

@devenbansod the adjustment I made in the test will work fine when errors are shown, the issue is that the coverage didn't account for my code change, that a success message should only be show when a database is selected

On that note, if it is true for a server import, I'll be happy to withdraw this PR. However, any time I attempt any import without a db selected, MySQL throws a 1046, that and previous issues leads me to believe it's a constant. So just let me know with this one, thanks :)

cc: @mauriciofauth

Contributor

ryanml commented Sep 23, 2017

@devenbansod the adjustment I made in the test will work fine when errors are shown, the issue is that the coverage didn't account for my code change, that a success message should only be show when a database is selected

On that note, if it is true for a server import, I'll be happy to withdraw this PR. However, any time I attempt any import without a db selected, MySQL throws a 1046, that and previous issues leads me to believe it's a constant. So just let me know with this one, thanks :)

cc: @mauriciofauth

Show outdated Hide outdated import.php
Checking that db does not equal an empty string
Signed-off-by: Ryan Lanese <rlanese@asu.edu>
@ryanml

This comment has been minimized.

Show comment
Hide comment
@ryanml

ryanml Oct 1, 2017

Contributor

@nijel @mauriciofauth I have pushed another commit again making the check ($db != '')

In any situation a database is not chosen, $db always contains an empty string.

This probably the best solution for what this PR is trying to accomplish, let me know what you guys think and if you'd like to move forward with this :)

Contributor

ryanml commented Oct 1, 2017

@nijel @mauriciofauth I have pushed another commit again making the check ($db != '')

In any situation a database is not chosen, $db always contains an empty string.

This probably the best solution for what this PR is trying to accomplish, let me know what you guys think and if you'd like to move forward with this :)

@ryanml

This comment has been minimized.

Show comment
Hide comment
@ryanml

ryanml Oct 2, 2017

Contributor

@nijel have you taken a look at this since my last commit or have any final thoughts on the issue in general?

Contributor

ryanml commented Oct 2, 2017

@nijel have you taken a look at this since my last commit or have any final thoughts on the issue in general?

@nijel

This comment has been minimized.

Show comment
Hide comment
@nijel

nijel Oct 3, 2017

Member

Honestly I'm not really sure what are you trying to achieve here. Why the server import should behave differently here?

Member

nijel commented Oct 3, 2017

Honestly I'm not really sure what are you trying to achieve here. Why the server import should behave differently here?

@mauriciofauth

This comment has been minimized.

Show comment
Hide comment
@mauriciofauth

mauriciofauth Oct 3, 2017

Member

It's related to #12856 and #13490.

Member

mauriciofauth commented Oct 3, 2017

It's related to #12856 and #13490.

@nijel

This comment has been minimized.

Show comment
Hide comment
@nijel

nijel Oct 3, 2017

Member

I think we should not show the success message on error, regardless it is on server, database or table. Not showing success message at all sounds confusing.

Member

nijel commented Oct 3, 2017

I think we should not show the success message on error, regardless it is on server, database or table. Not showing success message at all sounds confusing.

@ryanml

This comment has been minimized.

Show comment
Hide comment
@ryanml

ryanml Oct 3, 2017

Contributor

@nijel Sorry, I'm a little confused by your response, do you think we should, or should not show the success message? The standing issue was that the success message implies that a successful action has taken place

Contributor

ryanml commented Oct 3, 2017

@nijel Sorry, I'm a little confused by your response, do you think we should, or should not show the success message? The standing issue was that the success message implies that a successful action has taken place

@nijel

This comment has been minimized.

Show comment
Hide comment
@nijel

nijel Oct 3, 2017

Member

We should not show the success message if there was error during the import. The fact whether $db is set or not is really not relevant to that.

Member

nijel commented Oct 3, 2017

We should not show the success message if there was error during the import. The fact whether $db is set or not is really not relevant to that.

@nijel

This comment has been minimized.

Show comment
Hide comment
@nijel

nijel Oct 3, 2017

Member

To make it more explicit: The error handling is broken in all import situations, not only server wide.

screenshot-2017-10-3 localhost localhost gammu phpmyadmin 4 8 0-dev

Member

nijel commented Oct 3, 2017

To make it more explicit: The error handling is broken in all import situations, not only server wide.

screenshot-2017-10-3 localhost localhost gammu phpmyadmin 4 8 0-dev

@nijel

This comment has been minimized.

Show comment
Hide comment
@nijel

nijel Oct 3, 2017

Member

Sorry, but this is wrong approach to the issue (which was wrongly described as well). See #12856 (comment)

Member

nijel commented Oct 3, 2017

Sorry, but this is wrong approach to the issue (which was wrongly described as well). See #12856 (comment)

@nijel nijel closed this Oct 3, 2017

@nijel nijel self-assigned this Oct 3, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment