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

Broken scripts after v0.12.2 -> v0.17.0 upgrade #287

Closed
NuSkooler opened this issue Jul 24, 2017 · 7 comments
Closed

Broken scripts after v0.12.2 -> v0.17.0 upgrade #287

NuSkooler opened this issue Jul 24, 2017 · 7 comments

Comments

@NuSkooler
Copy link

NuSkooler commented Jul 24, 2017

First off, apologies for using an issue for questions - the Slack invite linked on k6.io states that it is no longer valid.

I have a working script created with v0.12.2. Upon upgrading to v0.17.0 there are three major issues:

  1. import { checkGet, checkPost, checkAuthenticate } from './general'; has to be changed to import { checkGet, checkPost, checkAuthenticate } from './general.js'; (note the .js)
  2. Object.assign now results in TypeError: Object has no member 'assign
  3. I had been creating a json_body member of res (code below). The reason for doing this vs using res.json() is it makes it convenient to have a default empty {} object so downstream checks can validate against it. With .json() those checks would fail. This now results in TypeError: Cannot assign to property json_body of a host object however. Example:
function getJsonBodyChecks(res, expectedContentType) {
	let jsonBodyCheck = {};
	
	if('application/json' === expectedContentType) {
		try {
			res.json_body = JSON.parse(res.body);
		} catch(e) {
		}
		jsonBodyCheck = {
			'body is JSON'	: (r) => 'object' === typeof(r.json_body),
		}
	}

	return jsonBodyCheck;
}

// ... down the line ...
export function checkGet(endpoint, additionalChecks = {}, expectedContentType='application/json' ) {
	const res = http.get(
		Endpoints[endpoint], 
		{
			headers : getBaseHeaders()
		}
	);

	const allChecks = Object.assign(
		{
			'status was 200'			: (r) => r.status === 200,
			'transaction time OK'		: (r) => r.timings.duration < 200,
			'content-type OK'			: (r) => r.headers['Content-Type'] && r.headers['Content-Type'].startsWith(expectedContentType),
		},
		getJsonBodyChecks(res, expectedContentType),
		additionalChecks
	);

	check(res, allChecks);

	return res;
}

Please advise on alternatives/workarounds!

@robingustafsson
Copy link
Member

Hi @NuSkooler. I've updated the Slack invite link now on k6.io: https://k6.io/slack/, sorry about that.

Regarding the issues you've encountered:

  1. Reading Cannot import local ES6 modules #198 (comment) I believe the reason for requiring the .js in more recent versions is a matter of opinion to make imports from local and remote sources as similar as possible (remote imports being a quite recent addition), but I'll let @liclac fill in on that. That it broke backwards compatibility is unfortunate.
  2. This is a known issue (CoreJS isn't loaded into the runtime #250). It's annoying and it's on the list of prioritized todos.

I've used the following snippet personally while waiting for Object.assign to become available again:

export function objectAssign(target, source) {
    var from;
    var to = target;

    for (var s = 1; s < arguments.length; s++) {
        from = Object(arguments[s]);

        for (var key in from) {
            if (from.hasOwnProperty(key)) {
                to[key] = from[key];
            }
        }
    }

    return to;
}
  1. I believe this is because of the switch of JS engine that happened as part of 0.13.0 (https://github.com/loadimpact/k6/releases/tag/v0.13.0). In this switch a lot of the APIs went from being pure JS APIs (including the HTTP Response class) to instead become Go objects/functions exposed through bindings to JS land. The switch was made for performance reasons.

I'm not sure I fully understand the reason for using res.json_body. When calling res.json() JSON parsing of res.body is invoked. This would result in an exception being thrown both in v0.12.2 and in v0.17.0 if the JSON is malformed. You could perhaps change getJsonBodyChecks to do something similar to:

function getJsonBodyChecks(res, expectedContentType) {
    let jsonBodyCheck = {};

    if ('application/json' === expectedContentType) {
        let wellformed = true;
        try {
            r.json();
        } catch(e) {
            wellformed = false;
        }
        jsonBodyCheck = {
            'body is JSON': (r) => wellformed,
        };
    }

    return jsonBodyCheck;
}

@NuSkooler
Copy link
Author

@robingustafsson Thank you very much for your quick response! The .js change makes sense and I look forward to the CoreJS fix!

I had a typo in my getJsonBodyChecks() method which is now updated (I had pasted it from code I was updating to use json() in attempts to get my script functioning again).

Perhaps this will illustrate why I've been using that approach:

export function checkPost(endpoint, payload, additionalChecks = {}, expectedContentType='application/json' ) {
	const res = http.post(
		Endpoints[endpoint], 
		JSON.stringify(payload), 
		{ 
			headers : getBaseHeaders()
		}
	);

	const allChecks = Object.assign(
		{
			'transaction time OK'		: (r) => r.timings.duration < 500,
			'content-type OK'			: (r) => r.headers['Content-Type'] && r.headers['Content-Type'].startsWith(expectedContentType),
		}, 
		getJsonBodyChecks(res, expectedContentType),
		additionalChecks
	);

	check(res, allChecks);

	return res;
}
// ...
export function checkAuthenticate(authPayload) {
	const res = checkPost(
		'oauth_token',
		authPayload,
		{
			'transaction time OK'		: (r) => r.timings.duration < 3000,	//	longer for auth
			'status was 201'			: (r) => r.status === 201,
			'acquired auth token'		: (r) => 'string' === typeof(r.json_body.auth_token),
			'acquired refresh token'	: (r) => 'string' === typeof(r.json_body.refresh_token),
			'ttl OK'					: (r) => r.json_body.expires_in >= 3600
		}
	);

	if(!res) {
		fail('failed to authenticate');
	}

	activeAuthInfo = res.json_body;

	return res;
}

What I get out of this vs using r.json() is the ability to do quick checks without knowing if the particular member is present (since json_body will always be at least an empty object {} )

If there is a better approach that is similar in it's lazy handed approach (DRY!), I'd love to hear it!

@NuSkooler
Copy link
Author

FWIW, I resorted to something like this until better advised:

function getJsonPayload(res) {
	let isJson;
	let payload = {};
	try {
		payload = res.json();
		isJson = true;
	} catch(e) {
		isJson = false;
	}

	return [ payload, isJson ];
}

export function checkJsonRpcCall(payload, addititionalChecks = {}) {
	const res = http.post(
		'https://some.sweet.host/restful_endpoint',
		JSON.stringify(payload),
		{
			'Content-Type' : 'application/json',
		}
	);

	const [jsonPayload, isJson] = getJsonPayload(res);

	const allChecks = Object.assign(
		{
			'is transaction time OK'		: (r) => r.timings.duration < 500,
			'is JSON response'				: () => isJson,
                        'is some JSON member OK'  : () => jsonPayload.someMember === 'OK',
		}
	);

	check(res, allChecks);

	return res;
}

@liclac
Copy link
Contributor

liclac commented Jul 31, 2017

Hm, from what I'm getting, you basically want to access a member in a JSON payload and not have to check if the path exists, right?

What if we added a function for doing that instead, like get(obj, "key.otherkey[1]") or so?

@NuSkooler
Copy link
Author

@liclac Basically yes.

Longer version:

  • Avoid json() conversion every time (This may not really be a issue of the engine caches it... I Haven't looked)
  • Don't want to try{}catch every time in the case of response wasn't actually JSON

I think having lodash / external libraries importing working well I could just _.get(obj, path)

@liclac
Copy link
Contributor

liclac commented Jul 31, 2017

Yeah, it's cached. I think a helper function may be the best way to go about it for if you don't wanna check if it's actually a JSON response, but imo we should offer a _.get() equivalent out of the box.

@liclac
Copy link
Contributor

liclac commented Aug 2, 2017

I'm closing this for now, could you open another issue for a get() function, referencing this one?

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