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

Adding a post hook that can run a measurement - try 2 #151

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

lcontami
Copy link
Member

@lcontami lcontami commented Jun 4, 2018

As requested in issue #17

lcontami added 5 commits June 4, 2018 09:59
# Conflicts:
#	docs/source/user_guide/measure_edition.rst
adding the methods to save/load the file
correcting the executions infos

# Conflicts:
#	docs/source/dev_guide/measurement.rst
@MatthieuDartiailh MatthieuDartiailh self-requested a review June 4, 2018 08:55
@MatthieuDartiailh
Copy link
Member

The Travis failing is probably related to some assumption done in the tests about the absence of more tools than the one considered. I have not looked at it in details yet.

@codecov
Copy link

codecov bot commented Jun 13, 2018

Codecov Report

Merging #151 into master will decrease coverage by 0.49%.
The diff coverage is 35.71%.

@@            Coverage Diff            @@
##           master     #151     +/-   ##
=========================================
- Coverage   98.38%   97.88%   -0.5%     
=========================================
  Files         157      158      +1     
  Lines       12293    12366     +73     
=========================================
+ Hits        12094    12104     +10     
- Misses        199      262     +63

"""Post-execusion hook to add a hierarchy of tasks.

"""
root_task = Typed(RootTask)
Copy link
Member

Choose a reason for hiding this comment

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

All those attributes should be documented using #: comments.

dependencies = Tuple()

def __init__(self, declaration, workbench):
""" Create an empty root task
Copy link
Member

Choose a reason for hiding this comment

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

No docstring for __init__ the class docstring is sufficient.

""" Widget used for the AddTaskHook

"""
attr hook
Copy link
Member

Choose a reason for hiding this comment

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

Same those deserve a #: comment

id = 'post-execution'
point = manifest.id + '.post-execution'
PostExecutionHook:
id = 'exopy.addtask_hook'
Copy link
Member

Choose a reason for hiding this comment

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

I must say I am not a big fan fan of this name... RunTaskHook perhaps ?

"""Create an AddTaskView.

"""
# on est obligé d'aller chercher dans le vrai manifest de measurement pour
Copy link
Member

Choose a reason for hiding this comment

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

In English please.

# assert hook.engine


def test_get_state(addtaskhook):
Copy link
Member

Choose a reason for hiding this comment

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

testing get_state and set_state can be combined to avoid painful manual checking of the config and manual construction of it.

"""
addtaskhook.engine = DummyEngine()
addtaskhook.stop(force=True)
assert addtaskhook.engine.stop_called == True
Copy link
Member

Choose a reason for hiding this comment

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

stop_called does not exist because it was not needed in other tests. You can either edit the dummy or use _stop

addtaskhook.stop(force=True)
assert addtaskhook.engine.stop_called == True

def test_view(addtaskview, addtaskhook):
Copy link
Member

Choose a reason for hiding this comment

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

Ideally we would like to show the widget to check it at least displays properly.

# ca devrait marcher pcq on appelle une seule fois la fixture, ensuite c'est le même objet
assert rootview.task == addtaskhook.root_task

def test_list_runtimes():
Copy link
Member

Choose a reason for hiding this comment

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

To test this the easiest way is to monkeypatch the infos of a task to add it a dependencies it does not have you can have a look at tests\tasks\test_dependencies.py.

Copy link
Member

@MatthieuDartiailh MatthieuDartiailh left a comment

Choose a reason for hiding this comment

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

I just looked at the changes not at Travis.

root_task = Typed(RootTask)

#: Reference to the measurement workbench
Copy link
Member

Choose a reason for hiding this comment

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

application workbench

The workbench is shared by the whole application and is not specific to the measurement plugin.

attr declaration

#: Reference to the corresponding measurement workbench
Copy link
Member

Choose a reason for hiding this comment

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

Same here

# 'task_id': 'DummyTask'}}
assert task_prefs # blabla selon sa structure

assert task_prefs # blabla selon sa structure ?
Copy link
Member

Choose a reason for hiding this comment

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

The comment can go away, no ?

Copy link
Member

@MatthieuDartiailh MatthieuDartiailh left a comment

Choose a reason for hiding this comment

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

I looked quickly at Travis failures and left some indications on how to fix them.

# on est obligé d'aller chercher dans le vrai manifest de measurement pour
# trouver le make_view
meas = measurement_workbench.get_plugin('exopy.measurement')
decl = meas.get_declarations('exopy.addtask_hook') # check how to get the declaration
Copy link
Member

Choose a reason for hiding this comment

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

You need to specify that 'exopy.addtask_hook' is a post_hook using the first argument of get_declarations.

"""
assert type(addtaskhook.root_task) == RootTask
assert type(addtaskhook.workbench) == Workbench
assert type(addtaskhook.dependencies) == Tuple
Copy link
Member

Choose a reason for hiding this comment

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

You are mixing the type of the decorator and the type of the value it returns. It should be tuple.

assert type(addtaskview.widget()[0]) == RootTaskView
rootview = addtaskview.widget()[0]
assert rootview.show_path == False
# ca devrait marcher pcq on appelle une seule fois la fixture, ensuite c'est le même objet
Copy link
Member

Choose a reason for hiding this comment

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

In english please.



@pytest.fixture
def addtaskhook(measurement):
Copy link
Member

Choose a reason for hiding this comment

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

You also need to make sure the TaskPlugin is registered. requesting the task_workbench should work.

Base automatically changed from master to main January 19, 2021 10:00
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