Ignore messages that matches a given pattern #502

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
6 participants

cc @jferris

This PR adds a new feature so that the user will be able to ignore messages from output by using a pattern. This is useful to get rid of annoying messages.

For example, after running the following code:

# ignore_pattern.rb

$LOAD_PATH.unshift "/vagrant/lib"
require "capybara"
require "capybara-webkit"

html = %(<html><body style="font-size: 0px">Hello World</body></html>)
app = proc { |env| [200, { "Content-Type" => "text/html" }, [html] ] }

sess = Capybara::Session.new(:webkit, app)
sess.visit("/")

You should see a warning "QFont::setPixelSize: Pixel size <= 0":

vagrant@precise32:/vagrant$ ruby ignore_pattern.rb 
QFont::setPixelSize: Pixel size <= 0 (0)
vagrant@precise32:/vagrant$

If you ask the driver to ignore it:

# ignore_pattern.rb

$LOAD_PATH.unshift "/vagrant/lib"
require "capybara"
require "capybara-webkit"

html = %(<html><body style="font-size: 0px">Hello World</body></html>)
app = proc { |env| [200, { "Content-Type" => "text/html" }, [html] ] }

sess = Capybara::Session.new(:webkit, app)
sess.driver.ignore("QFont::setPixelSize: Pixel size <= 0")
sess.visit("/")

Then you shouldn't see any QFont::setPixelSize: Pixel size <= 0 from the output:

vagrant@precise32:/vagrant$ ruby ignore_pattern.rb 
vagrant@precise32:/vagrant$ 

@jferris jferris and 1 other commented on an outdated diff Mar 29, 2013

spec/connection_spec.rb
@@ -32,6 +32,31 @@
io.string.should =~ /hello world $/
end
+ it 'ignores by a pattern' do
@jferris

jferris Mar 29, 2013

Owner

I think this would be easier to read/maintain as a Browser spec, because you can then rely on Browser to send/receive the command/response.

@jivagoalves

jivagoalves Mar 30, 2013

Sure, I'll try to come up with a better Browser spec instead.

@jferris jferris commented on an outdated diff Mar 29, 2013

spec/driver_spec.rb
@@ -2225,6 +2225,48 @@ def log
end
end
+ describe "ignoring messages" do
@jferris

jferris Mar 29, 2013

Owner

If we have a good Browser or Connection spec in place for this, I don't think we need to add a driver spec.

@jferris jferris and 1 other commented on an outdated diff Mar 29, 2013

src/IgnoreMessage.cpp
+#include "WebPageManager.h"
+#include <QRegExp>
+
+IgnoreMessage::IgnoreMessage(WebPageManager *manager, QStringList &arguments, QObject *parent) : SocketCommand(manager, arguments, parent) {
+}
+
+void IgnoreMessage::start() {
+ patterns << arguments()[0];
+ qInstallMsgHandler(messageHandler);
+ finish(true);
+}
+
+QStringList IgnoreMessage::patterns = QStringList();
+
+void IgnoreMessage::messageHandler(QtMsgType type, const char *msg) {
+ bool foundMatch = false;
@jferris

jferris Mar 29, 2013

Owner

I think extracting a shouldPrintMessage method from this would clarify this code.

@jivagoalves

jivagoalves Mar 30, 2013

Good point. I'm extracting it out.

Owner

jferris commented Mar 29, 2013

Thanks a bunch. We've wanted this feature for quite a while.

I had a few comments that I made inline, and I have one high-level question: what do you think about ignoring a few messages by default? For example, I'm assuming that "QFont" message isn't helpful to anybody using capybara-webkit, and it would save time and stress if it were ignored by default.

@mhoran thoughts?

Hey @jferris , thanks for reviewing. I think it's okay to ignore at least the well known message QFont::setPixelSize: Pixel size <= 0 (0). Since I don't know if there are other helpful QFont messages, I think it should be up to the users to ignore them. Unless you're pretty sure they're all worthless. Would you mind to list some messages you think it's not helpful?

@jferris I pushed some changes you've suggested:

  • More smaller methods to clarify the code
  • Browser spec instead

We can squash the commits later on when we agree everything is ready to be merged. Please let me know if there is anything we could improve. Thanks!

Collaborator

mhoran commented Mar 31, 2013

I wish there were a way to know the source of the message so we could just silence internal Qt errors. I definitely think we should have some default ignored messages, given all the open issues regarding debug output. But capturing those and keeping a list up to date is going to be tedious.

What if instead we install a message handler that ignores all debug and warning messages and instead of writing to qDebug from the logger, write to a QDebug instance that's tied to stderr? Then we get full control, and silence the internal errors without the need for a blacklist.

Another option would be a whitelist of messages instead of blacklist.

Owner

jferris commented Apr 1, 2013

If we could silence all internal errors in some way without keeping a list, that would certainly be nice.

Disabling the default qDebug instance would be a little annoying when we're actually debugging things, but I don't think it would be a deal-breaker.

We should also consider how we want console.log to behave; that's a useful debugging tool for users, but there are jQuery plugins and other 3rd party libraries that use console.log when it's defined. A number of users have complained about facebook messages in the output.

I think the changes here would be useful even if we could silence the internal Qt errors, given the issues with console.log.

Collaborator

mhoran commented Apr 1, 2013

As of 2ceab4e, console.log is written to the logger, so it will only appear on stderr when the debug driver is used.

@mhoran, would we have something like the following then?

void myMessageOutput(QtMsgType type, const char *msg)
 {
     switch (type) {
     case QtDebugMsg:
     case QtWarningMsg:
         break;
     default:
         fprintf(stderr, "%s\n", msg);
         break;
     }
 }

 int main(int argc, char **argv)
 {
     qInstallMsgHandler(myMessageOutput);
     QApplication app(argc, argv);
     ...
     return app.exec();
 }

I couldn't find how to return a QDebug instance tied to stderr. Can you please provide me some example/docs about that?

Besides that, I was thinking we could use two message handlers: one for ignoring debugs and warnings messages (the default handler above) and other to print to stderr. Therefore we would have to switch between them from the logger just to print at the moment we want. I'm not sure if that is the best solution though. What do you guys think?

Owner

jferris commented Apr 3, 2013

I think you can get a QDebug instance that writes to stderr with this:

QFile stderr;
stderr.open(STDERR)
QDebug debug(stderr);

Hey guys, sorry for the late response.

I've been trying to come up with a solution using a QDebug instance that writes to stderr without success.

// src/main.cpp
void messageHandler(QtMsgType type, const char *msg) {
  switch (type) {
    case QtDebugMsg:
    case QtWarningMsg:
      break;
    default:
      fprintf(stderr, "%s\n", msg);
      break;
  }
}

int main(int argc, char **argv) {
  qInstallMsgHandler(messageHandler);
  ...
}
// src/WebPageManager.cpp
QDebug WebPageManager::logger() const {
  if (m_loggingEnabled && m_stderrOutput->open(stderr, QIODevice::Append)) {
    return QDebug(m_stderrOutput);
  }
  else {
    return QDebug(m_ignoredOutput);
  }
}

After some testing it looks like I can't open the stderr file descriptor:

  2) Capybara::Webkit::Driver logger app logs its commands after turning on the logger
     Failure/Error: log.should include logging_message
       expected "" to include "Wrote response true"
     # ./spec/driver_spec.rb:2209:in `block (3 levels) in <top (required)>'

  3) Capybara::Webkit::Connection forwards stderr to the given IO object
     Failure/Error: io.string.should =~ /hello world $/
       expected: /hello world $/
            got: "" (using =~)
       Diff:
       @@ -1,2 +1 @@
       -/hello world $/
     # ./spec/connection_spec.rb:32:in `block (2 levels) in <top (required)>'

I did try another approach by switching the message handler when enabling the logger like the following:

// src/main.cpp
void messageHandler(QtMsgType type, const char *msg) {
  switch (type) {
    case QtDebugMsg:
    case QtWarningMsg:
      break;
    default:
      fprintf(stderr, "%s\n", msg);
      break;
  }
}

int main(int argc, char **argv) {
  qInstallMsgHandler(messageHandler);
  ...
}

and

// src/WebPageManager.cpp
void loggingMessageHandler(QtMsgType type, const char *msg) {
  fprintf(stderr, "%s\n", msg);
}

QDebug WebPageManager::logger() const {
  if (m_loggingEnabled) {
    qInstallMsgHandler(loggingMessageHandler);
    return qDebug();
  }
  else {
    return QDebug(m_ignoredOutput);
  }
}

That seems to work, but I don't know if it's a good solution though. What do you guys think? Do you know a better approach?

Owner

jferris commented Apr 19, 2013

Thanks for looking into this. A couple notes:

  • I'm not sure if qInstallMsgHandler applies just to the default qDebug instance, or to all QDebug instances globally.
  • The logger instance there is only used by capybara-webkit, and not by internal Qt messages. The warnings we're trying to silence will be writing to qDebug() whether or not m_loggingEnabled is true, so I think you want to install that handler unconditionally when capybara-webkit starts.

If you're having trouble with the approach I recommended, you could try out @mhoran's other idea: use a whitelist. Here's how I think it could work:

  • I think we only produce output when using logger. Any other output is from Qt.
  • Can we prefix our output with "capybara-webkit" somehow?
  • If so, we can install a message handler that ignores any messages that don't begin with "capybara-webkit."

Any updates on this?

brookr commented Sep 30, 2013

This would be super helpful for cleaning up test output... Warning messages are confusing to my students, and make them more likely to not read console output. :[

Owner

jferris commented Nov 7, 2013

@jivagoalves thanks for all your work on this. Because of the delay here, I ended up implementing this. Closing in favor of #586.

jferris closed this Nov 7, 2013

If anyone finds this thread and is still having problems with these warning messages in OS X Mavericks, please see: #581 (comment)

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