GNOME Bugzilla – Bug 685967
Support mocking of network answers
Last modified: 2012-11-13 15:44:47 UTC
Created attachment 226261 [details] [review] proposed patch Mock network answers of webservices through predefined files. This is useful for offline testing of plug-ins that provide sources from webservices. See the header of libs/net/grl-net-mock.c for full documentation.
Review of attachment 226261 [details] [review]: How about putting the documentation in a separate file so it can be installed along the rest of the devel files? It's also a shame that the output of the capture isn't usable for mocking the network responses. ::: libs/net/grl-net-mock.c @@ +26,3 @@ + * + * For configuring mock answers, a simple .ini file is used. The file is split + * into a "default" section and one section per URL. What format are the URLs in? I'm pretty certain that there's problem here with the allowed/escaped characters not being the same in ini files, and URIs. ::: libs/net/grl-net-mock.h @@ +28,3 @@ +#define GRL_ENV_NET_MOCKED "GRL_NET_MOCKED" + +#define GRL_NET_IS_MOCKED (g_getenv (GRL_ENV_NET_MOCKED)) This is counter-intuitive. GRL_ENV_NET_MOCKED=0 will return true. ::: libs/net/grl-net-private.c @@ +100,3 @@ + return; + + g_mkdir_with_parents (path, 0700); Check the retval to be sure that you're not generating loads of errors when the directory isn't created for example. @@ +120,3 @@ + escaped_uri = g_uri_escape_string (uri, NULL, FALSE); + g_free (uri); + file = g_strdup_printf ("%s"G_DIR_SEPARATOR_S"%s-%"G_GINT64_FORMAT, Construct the filename and the full path separately.
(In reply to comment #1) > Review of attachment 226261 [details] [review]: > It's also a shame that the output of the capture isn't usable for mocking the > network responses. That's slightly exaggerated. It gives you all the response files you need, you just need write the main ini file to wrap it together.
(In reply to comment #2) > (In reply to comment #1) > > Review of attachment 226261 [details] [review] [details]: > > > It's also a shame that the output of the capture isn't usable for mocking the > > network responses. > > That's slightly exaggerated. It gives you all the response files you need, you > just need write the main ini file to wrap it together. It still needs manual processing. I can't get a user to capture something for example, and be able to reuse it straight away. At the very least, I'd expect to run a script that prepares the ini file with the appropriate responses based on what was given.
(In reply to comment #3) > It still needs manual processing. I can't get a user to capture something for > example, and be able to reuse it straight away. Mocking surely isn't an use-case for end users...
Modifed the code as suggested and pushed the changes to the mocking branch for backup and convenience: http://git.gnome.org/browse/grilo/log/?h=mocking
Created attachment 226291 [details] [review] updated patch
(In reply to comment #4) > (In reply to comment #3) > > It still needs manual processing. I can't get a user to capture something for > > example, and be able to reuse it straight away. > > Mocking surely isn't an use-case for end users... It would be a great debugging tool for developers. We ask the users to capture their session, and we can reproduce errors exactly to fix them, and add them to a test suite in grilo-plugins (or wherever the plugins end up living).
Created attachment 226292 [details] [review] updated patch one more update: unthrottled mock sesssions. a bit boring to watch your unit tests throttling mocked connections for almost 2 minutes.
can i merge this?
Some comments: - I think we need to explain better the "ignored-parameters" key. The example of documentation seems to refers when we use "*". Another example explaining with one or two fields would be great. - Documentation suggests G_GNUC_INTERNAL should be defined before the function, not at the end (http://developer.gnome.org/glib/2.34/glib-Miscellaneous-Macros.html#G-GNUC-INTERNAL:CAPS) - Shows a message in case the dump directory can't be initialized, so user is aware dump won't take place. - Use GRL_NET_CAPTURE_DIR instead of GRL_WEB_CAPTURE_DIR. - GRL_NET_MOCKED is checked everytime an operation is done. I should be only checked once, when starting the application, so if we have it running and we define the variable it doesn't suddenly to mockup the results. - We should merge GRL_NET_MOCKED and GRL_REQUEST_MOCK_FILE in just once variable. That is, if GRL_NET_MOCKED is defined and pointing to a file, we would be using that file for mocking up the results. - Related with the above proposal, we should have GRL_NET_MOCK_THROTTLE containing the throttling time. Thus, 0 would mean no throttling at all, 1 means one second between requests, and so on. Of course, this variable would take effect only in case of mocking the results. - Can you rename grl-net-mock.h to grl-net-mock-private.h? In general we use the suffix "-private" for all the headers that contain private functions. So it is easier to know if tgrl-mock-data.inihe functions are public or not. - Rename grl-mock-data.ini to grl-net-mock-data.ini, to make it clear this data is for grl-net mocking. - Another suggestion: add the PID suffix in the .ini file, as grl-net-mock-data-%PID.ini. Thus, we can run several times the application and capture the data without loosing previous captures. - In the captured files, put the timestamp before the url. So it is easier to sort the results in chronological order.
Created attachment 226777 [details] [review] updated patch The branch is updated to address the comments. This patch reflects the aggregated changes in the branch.
Review of attachment 226777 [details] [review]: ::: doc/grilo/plugins-testing.xml @@ +110,3 @@ + To enable mocking set the environment variable GRL_NET_MOCKED. The value + of this variable is interpreted as colon separated list of configuration + settings, e.g. <code>config=mock-my-plugin.ini:throttle=0</code> While I understand that putting the throttle and the mocked file in the same envvar saves us to define new envar, I think it makes it a bit difficult to read/remember. But more carefully about it, I started to think that all this throttling stuff does not make sense when mocking up the results. The reason why we added the throttling option is not for a programming reason, but for fulfilling the Terms of Service of the service backend we use; thus, if the ToS says "do not make more than one request per second", then we set up the throttling to 1. But if you forget about this legal requirement, the plugin should work fine for any throttling value. When we use mocked results, we do not need to enfoce any legal ToS. Thus, we do not need to set up a throttling value. What is the summary of all of this? We should use always throttle==0 when mocking the results. So it wouldn't be needed to define it in the GRL_NET_MOCKED variable, using it just to tell the mock file to use (e.g., "export GRL_NET_MOCKED=~/mytests/mock/mock_my_plugin.ini")
Created attachment 226906 [details] [review] updated patch commit 670b93f12f3ad97179c7f7944f49aa410e4bee3b Author: Mathias Hasselmann <mathias@openismus.com> Date: Sat Oct 20 22:01:42 2012 +0200 net: Simplify GRL_NET_MOCKED variable Always set throttling to zero in mocking mode. commit eb86074842eb2c7368d2403dee264f9cc7e1185e Author: Mathias Hasselmann <mathias@openismus.com> Date: Sat Oct 20 21:55:38 2012 +0200 net: Always dipatch requests trough event loop This is to ensure consistent execution context for instant and for non-delayed web requests.
Created attachment 226993 [details] [review] updated patch commit d2e9d6a0a226240cf981d97d4a60b0f576aadf53 Author: Mathias Hasselmann <mathias@openismus.com> Date: Mon Oct 22 15:21:44 2012 +0200 net: Don't check GRL_NET_MOCKED for boolean values It's a pure filename now.
Created attachment 226995 [details] [review] updated patch commit 4a8bcff785198c7aa5ceee7f254e9b9045477804 Author: Mathias Hasselmann <mathias@openismus.com> Date: Mon Oct 22 15:33:57 2012 +0200 net: Update author tag commit 93928e515f585cf6235412f7ffbbeeb75b385abf Author: Mathias Hasselmann <mathias@openismus.com> Date: Mon Oct 22 15:28:38 2012 +0200 net: Don't fallback to "grl-net-mock-data.ini" Just print a warning and disable mocking if the specified mock response file cannot be found.
More updates came to branch. Now it is merged in upstream.
Is there any particular reason that we used environment variables (GRL_NET_MOCKED) for this? Why not use grl_config_set_api_key()? I guess that would make it easier to respond to changes, or initial setting, of GRL_NET_MOCKED. Currently, we need to call grl_registry_load_plugin_by_id() to re-load the plugin, to make it use a new GRL_NET_MOCKED value. And that causes warnings about re-registering grilo keys.
It would also be nice to have a way to discover when loading of the GRL_NET_MOCKED file has failed, or at least to detect that there was some failure. Maybe a grl_init() gboolean return, or a grl_init_succeeded() would be enough. Otherwise, tests can succeed just because the .ini file was not found, though a warning is shown.
(In reply to comment #17) > Is there any particular reason that we used environment variables > (GRL_NET_MOCKED) for this? Why not use grl_config_set_api_key()? I guess that > would make it easier to respond to changes, or initial setting, of > GRL_NET_MOCKED. I think this was already discussed. We use envvar instead GrlConfig because we want be able to use mocked content without changing any single line of the application. > > Currently, we need to call grl_registry_load_plugin_by_id() to re-load the > plugin, to make it use a new GRL_NET_MOCKED value. And that causes warnings > about re-registering grilo keys. The idea is that if you want to execute it with the mocked content, you must restart the application.
I think a warning is raised; if not, maybe it would be a good idea. Probably could be also a good idea to have a way of crashing on warnings/errors, similar to G_DEBUG=fatal_warnings.
(In reply to comment #20) > I think a warning is raised; if not, maybe it would be a good idea. > > Probably could be also a good idea to have a way of crashing on > warnings/errors, similar to G_DEBUG=fatal_warnings. This answer is for comment #18.